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
BUILD-264: Refresh-Resources Mode and Configurable Namespaces #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks like I would have expected @otaviof and in the spirit of our 1-1 kick off meeting / code walk through a while back... cool deal !!
A minor naming suggestion or two, otherwise just some in line observations I suspect are already on the path of resolution given you still have the WIP tag on this PR.
And for out of band observations, again with the qualifier that this is still WIP, is that as I'm sure you are already on path for, the unit/e2e stuff will need to be tweaked based on read only vs. read write.
But given your changes so far did not impact the e2e's run are certainly encouraging.
And I am not concerned that a few unit tests failed / need tweaks. Again, not surprising for a WIP PR.
FYI ... (re)confirmation that the CI changes from @adambkaplan and I are pulling in the image from this driver built from this PR's code. From the ci/prow/images job:
from one of the test e2e's:
same SHA for the "pipeline" imagestream There is an assumption in my statement above that https://github.com/openshift/csi-driver-shared-resource/blob/master/deploy/deploy.sh#L42-L45 worked. Certainly @otaviof as you move forward with this you are not entirely convinced, I would suggest starting in that deploy.sh file and cat'ing the contents of that csi-hostpath-plugin.yaml, along with temporary dumps of the csi driver pod logs for any new tracing you added. |
6a947e3
to
4716365
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I don't think that readonly
make sense as the flag name, maybe readonce
would be better?
3b4acea
to
c1838db
Compare
/retest |
@otaviof - as part of confirming that your I did a similar exercise back in #46 (comment) Now making another prelim pass at the code changes, still trying to take into account that you have the WIP marker on this PR and you are presumably not done making changes. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some prelim, the PR is still WIP, comments @otaviof - thanks
Okay, given the changes in As for the image replacement we use to tackle with $ curl --silent https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_csi-driver-shared-resource/46/pull-ci-openshift-csi-driver-shared-resource-master-e2e-aws-csi-driver/1430137437895528448/artifacts/e2e-aws-csi-driver/test/build-log.txt |grep -i 'patching.*image'
# Patching node-registrar image to use 'registry.build01.ci.openshift.org/ci-op-tv0f2inb/stable@sha256:b8e2d637f14e04658595393df59d99ac12284402c64c18c0a0aeecb29c70b1fb'
# Patching node-csi-driver image to use 'registry.build01.ci.openshift.org/ci-op-tv0f2inb/pipeline@sha256:b6ef81dc8d6b6b1714d0945402ba9e8ab508c6793096f21a21fb99653ac00689' The same image build by the CI: $ curl --silent https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_csi-driver-shared-resource/46/pull-ci-openshift-csi-driver-shared-resource-master-e2e-aws-csi-driver/1430137437895528448/artifacts/build-logs/csi-driver-shared-resource.log |grep 'b6ef81dc8d6b6b1714d0945402ba9e8ab508c6793096f21a21fb99653ac00689'
2021-08-24T12:01:42.821921844Z Successfully pushed image-registry.openshift-image-registry.svc:5000/ci-op-tv0f2inb/pipeline@sha256:b6ef81dc8d6b6b1714d0945402ba9e8ab508c6793096f21a21fb99653ac00689 Finally, inspecting the $ curl --silent https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_csi-driver-shared-resource/46/pull-ci-openshift-csi-driver-shared-resource-master-e2e-aws-csi-driver/1430137437895528448/artifacts/e2e-aws-csi-driver/gather-extra/artifacts/daemonsets.json |jq '.items[1].spec.template.spec.containers[1] | .args, .image'
[
"--drivername=csi.shared-resources.openshift.io",
"--v=4",
"--endpoint=$(CSI_ENDPOINT)",
"--nodeid=$(KUBE_NODE_NAME)",
"--ignorenamespace=openshift-machine-api",
/*** snip ***/
"--ignorenamespace=openshift-service-ca-operator"
]
"registry.build01.ci.openshift.org/ci-op-tv0f2inb/pipeline@sha256:b6ef81dc8d6b6b1714d0945402ba9e8ab508c6793096f21a21fb99653ac00689" Please consider. |
/retitle Populate Object Cache on Demand |
Awesome thanks @otaviof looks good /approve for now, between @coreydaley and I we can make the final / more detailed pass through the code and lgtm or post more comments, but seems like we are good to go |
deploy/deploy.sh
Outdated
@@ -16,54 +16,122 @@ | |||
set -e | |||
set -o pipefail | |||
|
|||
# when not empty it will run CSI driver on read-only mode | |||
READ_ONLY_MODE="${1}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will probably want to update this based on what we call the flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider latest changes.
/retitle BUILD-264: Refresh-Resources Mode and Configurable Namespaces |
Makefile
Outdated
# allows overwrite of node registrar image | ||
NODE_REGISTRAR_IMAGE ?= | ||
# allows overwite of CSI driver image | ||
DRIVER_IMAGE ?= | ||
# non-empty deploy mode will enable read-only mode deployment | ||
DEPLOY_MODE ?= | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.) Should these have a default specified?
2.) These should be at the top of the file so that anyone using it will know right away what options they have
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are variables previously used on deploy.sh
, now also exposed by Makefile
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DEPLOY_MODE
is new, and I'm updating the comment to make clear what's its objective. Let me know if it's clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment update looks good @otaviof
but I agree with @coreydaley that it should be moved toward the top, above all: ....
same with the TEST_SUITE and TEST_TIMEOUT that I think @adambkaplan recently added
thanks
fd0ad3e
to
1406188
Compare
/test e2e-aws-csi-driver-disruptive |
docs/content-update-details.md
Outdated
cache of `ConfigMap` and `Secret` objects, and as they change the controller will follow the updates. | ||
When `--refreshresources` is disabled (i.e. `--refreshresources=false`), the controller will read | ||
the backing-resource (`.spec.backingResource`) just before mounting the volume instead of keeping a | ||
warm cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good with the current implementation, but it reminded me of something. Something not explicitly called out in the Jira as I revisited it, but something I always had in mind when we first created this story.
Namely, if refreshresources is disabled, we force the volume to be read only.
- the code check would be around
csi-driver-shared-resource/pkg/hostpath/nodeserver.go
Lines 163 to 165 in 7c62db4
readOnly := req.GetReadonly() vol, err := ns.hp.createHostpathVolume(req.GetVolumeId(), kubeletTargetPath, readOnly, req.GetVolumeContext(), share, maxStorageCapacity, mountAccess) - I don't see why for the scenarios we are considering we would allow the consuming pod to change the contents we mounted, or add new files to our mount
- It defers the need of the atomic writer stuff from k8s/k8s as we would not have to ever coordinate with the pod
- remember, we claim to not be a "generic" csi volume provider, so ignoring user options like read only is a legitimate stance
@adambkaplan WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to highlight that when --refreshresources
is disabled, the data on the object-cache is refreshed just before the volume is mounted. Therefore, every time the volume is mounted the cache-key would get in sync with the apiserver again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have both "short living" pods like builds and will have "long running" pods which will be comsuming config maps with certs
in those long running cases, the volumes would only be mounted once
but while a good distinction to note @otaviof I think it is still orthogonal to the classic atomic write k8s secret stuff
there are 2 main motivators IMO for this feature's original creation
- scaling benefits from not employing a watch
- defaulting to read only to avoid atomic write hoopla when we know we won't update content after pod creation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gabemontero please consider the latest changes. Now on every volume mount when refresh-resources is not enabled, it will be a read-only volume.
Makefile
Outdated
# allows overwrite of node registrar image | ||
NODE_REGISTRAR_IMAGE ?= | ||
# allows overwite of CSI driver image | ||
DRIVER_IMAGE ?= | ||
# non-empty deploy mode will enable read-only mode deployment | ||
DEPLOY_MODE ?= | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment update looks good @otaviof
but I agree with @coreydaley that it should be moved toward the top, above all: ....
same with the TEST_SUITE and TEST_TIMEOUT that I think @adambkaplan recently added
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor ask, one suggested change that admittedly needs to be discussed, where I could see us going either way @otaviof and we need to discuss either here on or video conf with @adambkaplan
Also, I see the e2e commit for when refresh resources is disabled, and I see the unit test changes, but I was expecting e2e test changes in the go files where minimally the e2e tests that changed the share when refresh resources was disabled were not executed when we did test-e2e-no-refreshresources
per your Makefile update, as well as making sure content was not updated
i.e. a new TEST_SUITE
Were you planning those in a subsequent PR?
not sure why it removed approved .... some git intergration we don't have elsewhere /approve |
- --ignorenamespace=openshift-machine-api | ||
- --ignorenamespace=openshift-kube-apiserver | ||
- --ignorenamespace=openshift-kube-apiserver-operator | ||
- --ignorenamespace=openshift-kube-scheduler | ||
- --ignorenamespace=openshift-kube-controller-manager | ||
- --ignorenamespace=openshift-kube-controller-manager-operator | ||
- --ignorenamespace=openshift-kube-scheduler-operator | ||
- --ignorenamespace=openshift-console-operator | ||
- --ignorenamespace=openshift-controller-manager | ||
- --ignorenamespace=openshift-controller-manager-operator | ||
- --ignorenamespace=openshift-cloud-credential-operator | ||
- --ignorenamespace=openshift-authentication-operator | ||
- --ignorenamespace=openshift-service-ca | ||
- --ignorenamespace=openshift-kube-storage-version-migrator-operator | ||
- --ignorenamespace=openshift-config-operator | ||
- --ignorenamespace=openshift-etcd-operator | ||
- --ignorenamespace=openshift-apiserver-operator | ||
- --ignorenamespace=openshift-cluster-csi-drivers | ||
- --ignorenamespace=openshift-cluster-storage-operator | ||
- --ignorenamespace=openshift-cluster-version | ||
- --ignorenamespace=openshift-image-registry | ||
- --ignorenamespace=openshift-machine-config-operator | ||
- --ignorenamespace=openshift-sdn | ||
- --ignorenamespace=openshift-service-ca-operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this list going to have to get updated every time a new openshift-* namespace comes along? Couldn't we exclude all openshift-* namespaces with a regexp in the code somewhere if that is what needs to be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the rotating cert scenario we may need to watch certain openshift
namespaces
I at least am fine with deferring more sophisticated regex / other means of
expression in a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think we should take this in a standalone pull-request, I would like discuss this requirement further.
@@ -63,37 +68,11 @@ func NewController(shareRelist time.Duration) (*Controller, error) { | |||
// informers.NewSharedInformerFactoryWithOptions, but we restrict OpenShift | |||
// "system" namespaces with chatty configmaps like the leaderelection related ones | |||
// that are updated every few seconds | |||
//TODO make this list externally configurable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tweakListOptions := internalinterfaces.TweakListOptionsFunc(func(options *metav1.ListOptions) {
ignored := []string{}
for _, ns := range ignoredNamespaces {
klog.V(4).Infof("namespace '%s' is being ignored", ns)
ignored = append(ignored, fmt.Sprintf("metadata.namespace!=%s", ns))
}
options.FieldSelector = strings.Join(ignored, ",")
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be cleaner than all the appending strings stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true ... technically a tech debt item given @otaviof did not change that existing bit
I'm fine if he want to take that on now, or somebody does this as a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the heads up, @coreydaley, it's more readable this way. However, I would argue this change should be tackled in a subsequent PR, would be "a good first issue" by the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coreydaley I'm bringing that up on #60, thanks for the suggestion!
@gabemontero, in my view the I've tested this mode against all test suites successfuly. And on this PR we have unit-testing to assert the HostPathDriver works as expected when a client is informed: https://github.com/otaviof/csi-driver-shared-resource/blob/1406188d39046dc4de5a92c131682cadefcc7390/pkg/hostpath/hostpath_test.go#L544-L645 |
I'm not sure I agree it is orthogonal @otaviof , but I'll defer "officially" declaring that objection until I see more precisely what your openshift/release change is If I see something equivalent to both
I'll be satisfied. I did refresh my memory, and normal only deals with deletion of shares, so I agree its tests as is should work regardless of the refreshresources setting.
Yes I realize the tests are currently passing. |
Creating an additional flag (`--refreshresources`), so when disabled, the object-cache is updated just before mounting the volume. It allows the controller to only watch OpenShift's `Share`. Namespaces can now be configured with `--ignorenamespace` flag. Co-authored-by: Corey Daley <cdaley@redhat.com>
Adapting deploy.sh script with a flag to add an "refreshresources=false" mode, and introducing kustomize to edit the resources before rolling out in the cluster. Tweaking end-to-end test to wait for a configurable amount of pods produced by the `DeamonSet`, so it can run in smaller testing clusters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabemontero, otaviof 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 |
This pull-request adds two more flags on the operator (controller) part of the CSI Driver. And modifies the test infrastructure to be able to deploy with different arguments.
Flags
Using
--refreshresources=false
(default'strue
) flag the operator will only watchShare
resources, and will read the actual backing resource object just before mounting the volume. Previously it use to watch over most of theConfigMap
andSecret
resources. By using this flag the operator will consume less resources, and won't watch for eventual resources modifications.Additionally, the list of ignored namespaces can be edited using
--ignorenamespace
. The original list of ignored namespaces is kept by default.E2E
A new target is added,
test-e2e-no-refreshresources
which deploys the operator using this mode and runs the configured test-suite against. For instance: