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

adding vg api #402

Closed

Conversation

matancarmeli7
Copy link
Contributor

No description provided.

@mergify mergify bot added the vendor Pull requests that update vendored dependencies label Jun 28, 2023
@matancarmeli7 matancarmeli7 force-pushed the add_volume_group_controller branch 6 times, most recently from 4269cb3 to 54caffa Compare July 3, 2023 06:59
Copy link
Collaborator

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

There could be many more comments in the go-code. A description of the PR is missing, and would be nice to have references to the CSI/Kubernetes proposals.

controllers/volumegroup.storage/pkg/config/config.go Outdated Show resolved Hide resolved
@@ -82,4 +82,4 @@ require (
sigs.k8s.io/yaml v1.3.0 // indirect
)

replace github.com/csi-addons/spec => github.com/matancarmeli7/spec v0.0.0-20230703063652-bd35954246e7
replace github.com/csi-addons/spec => github.com/matancarmeli7/spec v0.0.0-20230711062057-d8d78175c552
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be part of this commit anymore once csi-addons/spec#54 is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, I will change it once the PR in the spec will be changed

Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be corrected now

controllers/volumegroup.storage/pkg/config/config.go Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from riya-singhal31 July 11, 2023 13:09
Signed-off-by: matancarmeli7 <[email protected]>
@nixpanic
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jul 28, 2023

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/1)
Auto-merging config/crd/bases/csiaddons.openshift.io_reclaimspacecronjobs.yaml
Auto-merging deploy/controller/crds.yaml
CONFLICT (content): Merge conflict in deploy/controller/crds.yaml
Auto-merging deploy/controller/install-all-in-one.yaml
CONFLICT (content): Merge conflict in deploy/controller/install-all-in-one.yaml
Auto-merging go.mod
Auto-merging go.sum
Auto-merging vendor/modules.txt
error: could not apply b808ddeb... adding vg api
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply b808ddeb... adding vg api

err-code: 35C31

Copy link
Collaborator

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

There most likely are functions under controllers/volumegroup.storage/utils that are available somewhere else, or would be suitable for other controllers too. This should probably be cleaned up at a later time.

At the moment I am missing how the communication to the CSI-driver is done. The kubernetes-csi-addons operator communicates over gRPC to the csi-addons-sidecar. The sidecar can be running alongside the CSI-controller (provisioner) or the CSI-nodeplugin. VolumeGroups most likely need to only call function of the CSI-controller. The protocol between the operator and sidecar should be rather simple as the sidecar can resolve the Kubernetes objects (PVC, PV, Secrets, ...). There is no .proto file added for this communication under https://github.com/csi-addons/kubernetes-csi-addons/tree/main/internal/proto , which is what I would expect.

continue
}

switch capType := cap.GetVolumeGroup().GetType(); capType {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe write this like

switch cap.GetVolumeGroup().GetType() {

There is no use for capType as additional variable.

@@ -82,4 +82,4 @@ require (
sigs.k8s.io/yaml v1.3.0 // indirect
)

replace github.com/csi-addons/spec => github.com/matancarmeli7/spec v0.0.0-20230703063652-bd35954246e7
replace github.com/csi-addons/spec => github.com/matancarmeli7/spec v0.0.0-20230711062057-d8d78175c552
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be corrected now

@nixpanic
Copy link
Collaborator

This is too complex to include, #588 is a simpler API that lets CSI-drivers handle the grouping of volumes how they like.

@nixpanic nixpanic closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vendor Pull requests that update vendored dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants