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

wants to merge 5 commits into from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Feb 2, 2023 •
edited
Loading

Description

See #229

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

lfrancke
lfrancke reviewed Feb 2, 2023
View reviewed changes
deploy/helm/secret-operator/templates/daemonset.yaml
@@ -69,7 +69,7 @@ spec:
volumes:
- name: registration-sock
hostPath:
path: /var/lib/kubelet/plugins_registry/secrets.stackable.tech-reg.sock
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.

@nightkr nightkr requested a review from lfrancke February 2, 2023 11:33
lfrancke
lfrancke reviewed Feb 2, 2023
View reviewed changes
CHANGELOG.md
@@ -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.

@lfrancke lfrancke added the release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. label Feb 2, 2023
@lfrancke
Copy link
Member

lfrancke commented Feb 2, 2023

I labeled this as "breaking" because we don't have a better label this second.
We should mention this change in the next release notes.

lfrancke
lfrancke approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@lfrancke lfrancke left a comment

Choose a reason for hiding this comment

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

I'll approve it. Not sure if anyone else wants to have a look.

@nightkr nightkr requested a review from a team February 2, 2023 11:52
@nightkr
Copy link
Member Author

nightkr commented Feb 2, 2023

Ugh, called it too early, there seem to be more problems involved for microk8s...

@nightkr nightkr removed the request for review from a team February 2, 2023 11:55
@nightkr nightkr changed the title Shorten the CSI registration path Fix microk8s compatibility Feb 2, 2023
@nightkr nightkr changed the title Fix microk8s compatibility Shorten the CSI registration path Feb 2, 2023
@nightkr
Copy link
Member Author

nightkr commented Feb 2, 2023

I'll leave this for just the path length issue, and submit a separate PR for the rest...

sbernauer
sbernauer approved these changes Feb 2, 2023
View reviewed changes
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Thx for fixing this! Looking forward to the other change

@nightkr
Copy link
Member Author

nightkr commented Feb 2, 2023

bors r+

bors bot pushed a commit that referenced this pull request Feb 2, 2023
@nightkr
Shorten the CSI registration path (#231)
d219e79 
# Description

See #229
@bors
Copy link
Contributor

bors bot commented Feb 2, 2023

Build failed:

@nightkr
Copy link
Member Author

nightkr commented Feb 2, 2023

bors retry

bors bot pushed a commit that referenced this pull request Feb 2, 2023
@nightkr
Shorten the CSI registration path (#231)
172ad83 
# Description

See #229
@bors
Copy link
Contributor

bors bot commented Feb 2, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Shorten the CSI registration path [Merged by Bors] - Shorten the CSI registration path Feb 2, 2023
@bors bors bot closed this Feb 2, 2023
@bors bors bot deleted the bugfix/too-long-csi-reg-path branch February 2, 2023 12:58
@nightkr nightkr mentioned this pull request Feb 2, 2023
Closed
@nightkr nightkr mentioned this pull request Mar 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lfrancke lfrancke lfrancke approved these changes

@sbernauer sbernauer sbernauer approved these changes

Assignees
No one assigned
Labels
release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nightkr @lfrancke @sbernauer