-
Notifications
You must be signed in to change notification settings - Fork 34
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 projected token volume for hostNetwork pods. #428
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,4 +117,4 @@ spec: | |
- name: metrics | ||
protocol: TCP | ||
port: 8080 | ||
targetPort: 22032 | ||
targetPort: 22032 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If this goes inside mounter.Mount, do we need this flag? I think we pass (or can pass) hostNetwork flag through mountConfig.
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 can simplify the sidecar injection logic too.
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 still need this to pass to mount config
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.
You are using the new HostNetwork flag to:
To avoid creating flags, we can determine if the Pod supports this feature on
gcs-fuse-csi-driver
container by checking ifhostNetwork
is enabled on the pod and the volume we need is injected. We have a pod informer that has that information.gcs-fuse-csi-driver/pkg/csi_driver/node.go
Line 137 in e5d8871
We can then pass the mountConfig from
gcs-fuse-csi-driver
->gke-gcsfuse-sidecar
driver sending mc to sidecar:
gcs-fuse-csi-driver/pkg/csi_mounter/csi_mounter.go
Line 86 in e5d8871
sidecar receiving mc from driver:
gcs-fuse-csi-driver/pkg/sidecar_mounter/sidecar_mounter_config.go
Line 129 in e5d8871
After that, we can process mc to set the configmap with the uds path
gcs-fuse-csi-driver/pkg/sidecar_mounter/sidecar_mounter_config.go
Line 151 in e5d8871
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.
Hey Jaime, thanks for the suggestion. Somehow the resulting pod of k8sclients.GetPod(...) does not persist the HostNetwork value. It is "false" despite my test pod setting. It might be worthwhile to derive the HostNetwork from
pod.Annotations["kubectl.kubernetes.io/last-applied-configuration"]
, but it might be very messy.This is the last-applied annotation.
{"apiVersion":"v1","kind":"Pod","metadata":{"annotations":{"gke-gcsfuse/volumes":"true"},"name":"test-pv-pod-hnw","namespace":"ns1"},"spec":{"containers":[{"command":["sleep","3600"],"image":"busybox","name":"busybox","volumeMounts":[{"mountPath":"/data","name":"gcp-cloud-storage-pvc"},{"mountPath":"/dataEph","name":"gcs-fuse-csi-ephemeral"}]}],"hostNetwork":true,"serviceAccountName":"test-ksa-ns1","volumes":[{"name":"gcp-cloud-storage-pvc","persistentVolumeClaim":{"claimName":"gcp-cloud-storage-csi-static-pvc"}},{"csi":{"driver":"gcsfuse.csi.storage.gke.io","volumeAttributes":{"bucketName":"test-wi-host-network-2","mountOptions":"implicit-dirs"}},"name":"gcs-fuse-csi-ephemeral"}]}}
So far the cleanest and safest solution is passing the bool flag from webhook. What do you think? Let me know.
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.
Synced with Jaime offline. PodSpec was filtered in this PR: #413
This approach proves to be unfeasible even after adding back hostNetwork in podSpec. In csi driver, the msg is sent to the listner in the csi path, that looks like "/var/lib/kubelet/pods/c9a5236b-cf41-49c0-bcfe-f61988440b8f/volumes/kubernetes.io~csi/gcs-fuse-csi-ephemeral/mount". The msg received in sidecar mounter is received in the path of the volume socket: "connecting to socket "/gcsfuse-tmp/.volumes/gcs-fuse-csi-ephemeral/socket". They are not the same msg.
Will stick to the original design.
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.
@siyanshen Do we know how all the other options are making their way from the node-server to the sidecar? Regarding the different paths being used, I believe we are using a symbolic link for socket communication. Could you clarify this so going forward we follow a different approach then?
gcs-fuse-csi-driver/pkg/csi_mounter/csi_mounter.go
Lines 211 to 215 in eeeb147