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

controller: add disk tags to BlockDeviceSpec #60

Merged

Conversation

Vicente-Cheng
Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng commented Nov 30, 2023

Problem:
We want to add tags when we add extra disks.
(We need to add tags after we provision the disk )

Solution:
Add Tags to BlockDeviceSpec, then the controller can add it when provisioning

Related Issue:
harvester/harvester#2717

Test plan:

  • Add disk tags with provision, e.g. (blockdevices CRD)
...
spec:
  devPath: /dev/sda
  fileSystem:
    forceFormatted: true
    mountPoint: ""
    provisioned: true
  nodeName: harvester-node-0
  tags:
  - default
  - ssd
...
  • Check the nodes.longhorn.io. The related disk should have the same tags.
  • Add/Remove tags with provisioned blockdevices

Tasks

  • blockdevices controller should handle tag anytime
  • add node watcher to sync tags
  • add CI for it

@Vicente-Cheng Vicente-Cheng marked this pull request as draft November 30, 2023 10:00
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from bbedf62 to ff30429 Compare December 4, 2023 11:47
@Vicente-Cheng Vicente-Cheng mentioned this pull request Dec 4, 2023
@Vicente-Cheng Vicente-Cheng requested a review from bk201 December 4, 2023 15:41
@Vicente-Cheng Vicente-Cheng marked this pull request as ready for review December 4, 2023 15:44
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch 2 times, most recently from 1d25abf to 6fb818d Compare December 4, 2023 16:36
@Vicente-Cheng
Copy link
Collaborator Author

Need new Longhorn deployment script, depends on harvester/vagrant-rancherd#8

@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch 6 times, most recently from daef788 to 5dc1578 Compare December 5, 2023 09:01
@bk201 bk201 requested a review from tserong December 5, 2023 09:25
Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

Just to make sure I understand, this is meant to take tags defined via either harvester or longhorn, and add them to the block device spec, and then to the block device status. If so, it looks like it should generally do the right thing, I just have a couple of comments.

Commit 25afa25 ("controller: add disk tags to BlockDeviceSpec") does two things - it updates go.mod etc. with new/updated dependencies, and it also makes those changes to the controller. I'd suggest splitting that into two separate commits, one for the dependency update, and one for the main code change.

pkg/controller/blockdevice/controller.go Show resolved Hide resolved
pkg/controller/blockdevice/controller.go Outdated Show resolved Hide resolved
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from 5dc1578 to cf85698 Compare December 6, 2023 07:44
@Vicente-Cheng
Copy link
Collaborator Author

Just to make sure I understand, this is meant to take tags defined via either harvester or longhorn, and add them to the block device spec, and then to the block device status. If so, it looks like it should generally do the right thing, I just have a couple of comments.

Blockdevices is the CR for the harvester, longhorn directly managed the disk with longhorn.io/v1beta2/nodes CR.
So here we only focus on the harvester Blockdevices CR but any change in the longhorn.io/v1beta2/nodes related to harvester Blockdevices CR will be synced.

Commit 25afa25 ("controller: add disk tags to BlockDeviceSpec") does two things - it updates go.mod etc. with new/updated dependencies, and it also makes those changes to the controller. I'd suggest splitting that into two separate commits, one for the dependency update, and one for the main code change.

Good suggestions, I split another commit for it. That will make the review easier. Thanks for the suggestion.

@Vicente-Cheng Vicente-Cheng requested a review from tserong December 6, 2023 07:51
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from 78bb1b8 to dea6895 Compare December 6, 2023 08:28
Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

LGTM

@tserong tserong self-requested a review December 7, 2023 01:50
Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

Oops, no, wait a sec - I read the code but didn't check the tests, one of which has failed: https://github.com/harvester/node-disk-manager/actions/runs/7112001697/job/19361613496?pr=60

@Vicente-Cheng
Copy link
Collaborator Author

Oops, no, wait a sec - I read the code but didn't check the tests, one of which has failed: https://github.com/harvester/node-disk-manager/actions/runs/7112001697/job/19361613496?pr=60

This failed because longhorn.io/v1beta2/nodes updated too frequently (means we will provision failed and queue again), CI does not know it. I am working on it.

@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch 2 times, most recently from e562c5e to 6f9e796 Compare December 13, 2023 08:53
@Vicente-Cheng
Copy link
Collaborator Author

Depends on harvester/go-common#8
I moved some general functions to the above repo. Once it is merged, I will update the PR again.

BTW, CI is also fixed now.

@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from 6f9e796 to da5036b Compare December 14, 2023 10:22
@Vicente-Cheng
Copy link
Collaborator Author

Hi @tserong, Please help recheck this PR.
Thanks!

Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

LGTM!

@Vicente-Cheng Vicente-Cheng mentioned this pull request Dec 20, 2023
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch 3 times, most recently from 28a192d to a4b5452 Compare December 22, 2023 07:04
    - Also bump golangci-lint to v1.55.2

Signed-off-by: Vicente Cheng <[email protected]>
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from a4b5452 to 06cd1f0 Compare January 4, 2024 01:52
pkg/controller/node/node.go Outdated Show resolved Hide resolved
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from 71c2e5c to d206949 Compare January 8, 2024 19:16
@Vicente-Cheng Vicente-Cheng requested a review from bk201 January 9, 2024 02:18
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch 2 times, most recently from 621f73f to 58b3658 Compare January 9, 2024 13:11
Signed-off-by: Vicente Cheng <[email protected]>
    - for slice related functions

Signed-off-by: Vicente Cheng <[email protected]>
    - we do not need the long jitter

Signed-off-by: Vicente Cheng <[email protected]>
    Now, the BlockDevices CR will handle its disk Tags. These tags
    will deploy to the Longhorn node.

    Some related fields,
    - Blockdevice.Spec.Tags: Harvester defined disk Tags
    - Blockdevice.Status.Tags: disk Tags from longhorn nodes and
      Harvester defined
    - disk.Tags of nodes.longhorn.io: disk Tags defined by both
      harvester and longhorn

    - change the variable name from needUpdated -> updated, that
      will make it more readable.

Signed-off-by: Vicente Cheng <[email protected]>
    - deploy longhorn v1.5.3
    - update the deployment CRD
    - Add tags with provision
    - Remove tags from provisioned blockdevice
    - Add tags to provisioned blockdevice
    - Add more info with upgrade operation

Signed-off-by: Vicente Cheng <[email protected]>
@Vicente-Cheng Vicente-Cheng force-pushed the add-disk-tags-to-controller-provisioner branch from 58b3658 to 3b62c68 Compare January 9, 2024 14:03
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bk201 bk201 merged commit 44f601c into harvester:master Jan 19, 2024
8 checks passed
@Vicente-Cheng
Copy link
Collaborator Author

@Mergifyio backport v0.6.x

Copy link

mergify bot commented Jan 19, 2024

backport v0.6.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants