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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Shorten the CSI registration path #231

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Shorten the CSI registration path #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a762d77
Shorten the CSI registration path (fixes #229)
nightkr Feb 2, 2023
33c1ffe
Merge branch 'main' into bugfix/too-long-csi-reg-path
nightkr Feb 2, 2023
4cc0403
Comment on why this is still safe
nightkr Feb 2, 2023
ac7ea1c
CHANGELOG
nightkr Feb 2, 2023
29bc261
Add link to issue
nightkr Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,13 @@ All notable changes to this project will be documented in this file.
### Changed

- operator-rs: 0.25.0 -> 0.27.1 ([#212]).
- Shortened the registration socket path for Microk8s compatibility ([#231]).
- After upgrading you will need to
`rmdir /var/lib/kubelet/plugins_registry/secrets.stackable.tech-reg.sock` manually.
Copy link
Member

@lfrancke lfrancke Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What happens if you don't?

I assume it'll just be unused but not dangerous?

Copy link
Member Author

@nightkr nightkr Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

secret-operator will fail to start back up until it's done (or rather, secret-op itself will start, but the node-driver-registrar sidecar will fail).

Copy link
Member

Choose a reason for hiding this comment

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

Oh! Well, then my breaking change label is correct after all.....

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly, yes.

This applies to *all* users, not just Microk8s.

[#212]: https://github.com/stackabletech/secret-operator/pull/212
[#231]: https://github.com/stackabletech/secret-operator/pull/231

## [0.6.0] - 2022-11-07

Expand Down
4 changes: 3 additions & 1 deletion deploy/helm/secret-operator/templates/daemonset.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ spec:
volumes:
- name: registration-sock
hostPath:
path: /var/lib/kubelet/plugins_registry/secrets.stackable.tech-reg.sock
# node-driver-registrar appends a driver-unique filename to this path to avoid conflicts
# see https://github.com/stackabletech/secret-operator/issues/229 for why this path should not be too long
path: /var/lib/kubelet/plugins_registry
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about this but...did it point to a directory before as well?

And is there any chance of a namespace clash?

And...does YAML support comments? If yes...this'd be a great place for a link to the issue...

Copy link
Member Author

Choose a reason for hiding this comment

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

The node-registrar is hard-coded to use $REGISTRATION_SOCK_PATH/$CSI_DRIVER-reg.sock. This lead to the actual path being /var/lib/kubelet/plugins_registry/secrets.stackable.tech-reg.sock/secrets.stackable.tech-reg.sock rather than the correct /var/lib/kubelet/plugins_registry/secrets.stackable.tech-reg.sock.

In most cases it still worked fine, because kubelet scans the plugin registry path recursively... except for that it can't connect when the path gets too long (#229).

I can add a comment about it, sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being pedantic and I do know git blame exists but I find it incredibly helpful if code links to issues

# node-driver-registrar appends a driver-unique filename to this path to avoid conflicts
# see https://github.com/stackabletech/secret-operator/issues/229 for why this path should not be too long

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

- name: csi
hostPath:
path: /var/lib/kubelet/plugins/secrets.stackable.tech/
Expand Down