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

set mount type to DirectoryOrCreate for hostPath needed by Calico #3349

Merged
merged 1 commit into from
May 10, 2024

Conversation

mazdakn
Copy link
Member

@mazdakn mazdakn commented May 8, 2024

Description

Fixes: projectcalico/calico#8773

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@@ -1082,7 +1082,7 @@ func (c *nodeComponent) nodeVolumes() []corev1.Volume {
if c.cfg.Installation.CNI.Type == operatorv1.PluginCalico {
// Determine directories to use for CNI artifacts based on the provider.
cniNetDir, cniBinDir, cniLogDir := c.cniDirectories()
volumes = append(volumes, corev1.Volume{Name: "cni-bin-dir", VolumeSource: corev1.VolumeSource{HostPath: &corev1.HostPathVolumeSource{Path: cniBinDir}}})
volumes = append(volumes, corev1.Volume{Name: "cni-bin-dir", VolumeSource: corev1.VolumeSource{HostPath: &corev1.HostPathVolumeSource{Path: cniBinDir, Type: &dirOrCreate}}})
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 curious why we need this on the cni bin dir, but not the other CNI volume mounts as well?

Copy link
Member Author

@mazdakn mazdakn May 8, 2024

Choose a reason for hiding this comment

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

I think in this case, the user reporter these 3 dirs as they hit the error with these 3 ones. However, in other setups another user might hit the issue with the other paths. Now, we can either fix these 3 as we know the issue has happened and wait for other incidents, or we can add this property for all directories that we think might be affected.
It's good to be proactive and set all the really needs it, but it might needs a bit more investigation to realise which ones really needs it. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

btw, the same question applies to calico node mounts. Maybe we should update all of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

@caseydavenport I don't know about the purpose of these directories. Given you know more about Calico CNI, does it make sense to set the same Type to the other directories? If so, I'll add it.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose let's leave them as is for now just to keep the changes minimal to fix the originally reported issue. I suspect these probably suffer the same problem in some cases, but we've never heard about 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.

Helm chart assumes various hostPath volumes are existing directories
3 participants