Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use an alternative CNI lock for read-only config dirs #1122

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

mheon
Copy link
Member

@mheon mheon commented Aug 17, 2022

When the configuration directory is on a read-only filesystem, there's no risk of concurrency issues as there's no possibility of changing anything. As such, while it prevents the use of our default lock location, it also removes any need for a lock at all.

Making the lock entirely optional is a lot of code, so instead of doing that let's just put it in our temporary files directory, where it can't hurt anything.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member Author

mheon commented Aug 17, 2022

Please don't merge yet, validating in Podman to ensure this works as expected.

@mheon mheon force-pushed the alt_lock_on_errofs branch from e00abda to cc3e192 Compare August 17, 2022 18:42
@mheon
Copy link
Member Author

mheon commented Aug 17, 2022

Good to merge, verified this works with Podman

When the configuration directory is on a read-only filesystem,
there's no risk of concurrency issues as there's no possibility
of changing anything. As such, while it prevents the use of our
default lock location, it also removes any need for a lock at
all.

Making the lock entirely optional is a lot of code, so instead of
doing that let's just put it in our temporary files directory,
where it can't hurt anything.

Signed-off-by: Matthew Heon <mheon@redhat.com>
@mheon mheon force-pushed the alt_lock_on_errofs branch from cc3e192 to 50c2c97 Compare August 17, 2022 18:59
@mheon
Copy link
Member Author

mheon commented Aug 17, 2022

CI does not seem to have realized I pushed a fresh commit... Github seems to be in a bad way.

@mheon
Copy link
Member Author

mheon commented Aug 17, 2022

CI is failing trying to download a package from Github. Probably related to the issues GH is having this afternoon?

@mheon
Copy link
Member Author

mheon commented Aug 17, 2022

@containers/podman-maintainers PTAL

@baude
Copy link
Member

baude commented Aug 17, 2022

lgtm, if you end up repushing, mind added the bz #?

}
} else {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably overthinking this. But if you establish a lock file in the RunDir due to the CNIConfigDir being readonly, and then that file in CNIConfigDir becomes writeable, could we end up with two lock files, and what does that mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future Podman invocations will use the new read-write lock, so everything should be fine.

@TomSweeneyRedHat
Copy link
Member

@mheon
Copy link
Member Author

mheon commented Aug 18, 2022

Tests green, should be good to merge

@baude
Copy link
Member

baude commented Aug 18, 2022

/lgtm

@Luap99
Copy link
Member

Luap99 commented Sep 5, 2022

@mheon I think we would need the same for the netavark backend as well?

@mheon
Copy link
Member Author

mheon commented Sep 6, 2022

@Luap99 Only requested for CNI for now... Frankly, I don't know how sane a read-only network directory is, so I'm a little hesitant to keep making changes related to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants