[WIP-Refactor part 1] Move container mounts to internal/factory/container #7859
Conversation
Signed-off-by: Amit Kumar <kramit6662@gmail.com>
Signed-off-by: T K Chandra Hasan <t.k.chandra.hasan@ibm.com>
openshift-ci
bot
added
release-note-none
openshift-ci
bot
added
the
kind/cleanup
openshift-ci
bot
added
the
kind/other
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hasan4791 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
UTs are still in progress. |
hasan4791
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err == nil && currentLabel == canonicalSecLabel { | ||
| logrus.Debugf( | ||
| "Skipping relabel for %s, as TrySkipVolumeSELinuxLabel is true and the label of the top level of the volume is already correct", | ||
| path) |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
|
@dfr FYI as you're working on freebsd changes!!! |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@hasan4791: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| } | ||
|
|
||
| // SpecAddPostOCIMounts add mounts after creating ocicontainer | ||
| func (c *container) SpecAddPostOCIMounts(ctx context.Context, serverConfig *sconfig.Config, containerInfo storage.ContainerInfo, ociContainer *oci.Container, mountPoint string, timeZone string, rootPair idtools.IDPair) error { |
There was a problem hiding this comment.
So this doesn't add mounts to the spec, it directly changes the rootfs. Maybe consider changing the name to reflect this?
| @@ -0,0 +1,58 @@ | |||
| FROM fedora:latest | |||
There was a problem hiding this comment.
what is this included for?
There was a problem hiding this comment.
Please ignore this file, currently im running integration tests in container locally and using this file.
| testunit-package: | ||
| mkdir -p ${TESTBIN_PATH} | ||
| go test ${PACKAGE} \ | ||
| --tags "test $(BUILDTAGS)" \ | ||
| --gcflags '-N' -c -o ${TESTBIN_PATH}/$$(basename ${PACKAGE}) | ||
|
|
There was a problem hiding this comment.
this can be acheieved by doing TESTFLAGS='--focus="package' make testunit
| package container | ||
|
|
||
| // Impl is the main implementation interface of this package. | ||
| type Impl interface { | ||
| SecurityLabel(path, secLabel string, shared, maybeRelabel bool) error | ||
| } |
There was a problem hiding this comment.
I don't think this is needed if you just add a label_unsupported.go file that defines SecurityLabel as an empty function
| func clearMountInfo(c *container) { | ||
| c.mountInfo.criMounts = nil | ||
| c.mountInfo.mounts = nil | ||
| c.mountInfo = nil | ||
| } |
There was a problem hiding this comment.
why not leave it for the duration of the container creation process? the container factory won't live that long
| // SpecAddPreOCIMounts add mounts to the spec before creating ocicontainer | ||
| func (c *container) SpecAddPreOCIMounts(ctx context.Context, resourceStore *resourcestore.ResourceStore, serverConfig *sconfig.Config, sb *sandbox.Sandbox, containerInfo storage.ContainerInfo, mountPoint string, idMapSupport bool) ([]oci.ContainerVolume, []rspec.Mount, error) { | ||
| // Create temp mountInfo | ||
| c.mountInfo = newMountInfo() |
There was a problem hiding this comment.
do we need to save mountInfo into the container if we're not referencing it after this function?
| // Clear temp mountInfo | ||
| defer clearMountInfo(c) |
There was a problem hiding this comment.
if we clear mountinfo between these two calls (pre and post) then we lose the deduplication right?
|
A friendly reminder that this PR had no activity for 30 days. |
github-actions
bot
added
the
lifecycle/stale
sohankunkerkar
removed
the
lifecycle/stale
What type of PR is this?
/kind api-change
/kind ci
/kind cleanup
/kind other
What this PR does / why we need it:
This PR is part-1 of container creation code refactoring where the mount related codes are moved to internal/factory/container/mounts file.
Which issue(s) this PR fixes:
Fixes #7275
Special notes for your reviewer:
Does this PR introduce a user-facing change?