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

feat: implementation of topolvm controller resource unit #21

Merged
merged 3 commits into from
Dec 17, 2021
Merged

feat: implementation of topolvm controller resource unit #21

merged 3 commits into from
Dec 17, 2021

Conversation

leelavg
Copy link
Contributor

@leelavg leelavg commented Dec 13, 2021

  • implemented create and delete reconcile actions for TopolvmController
  • added rbac roles to manager for manage TopolvmController resource
  • added rbac roles for topolvm-controller to manage CSI Controller functionanity

Signed-off-by: Leela Venkaiah G lgangava@redhat.com

nbalacha
nbalacha suggested changes Dec 13, 2021
View reviewed changes
Copy link
Contributor

@nbalacha nbalacha left a comment

Choose a reason for hiding this comment

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

Please split this PR into multiple commits. (code + manifests)

controllers/lvmcluster_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role_bindings.yaml Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role.yaml Outdated Show resolved Hide resolved
controllers/defaults.go Outdated Show resolved Hide resolved
config/default/kustomization.yaml Show resolved Hide resolved
@leelavg
Copy link
Contributor Author

leelavg commented Dec 14, 2021

  • tested against a live cluster by deploying, updating and deleting lvmcluster CR and observing reconciliation of topolvm controller resource unit
  • deployed csi driver manually as that's part of another PR and this resource depends on csidriver presence

This was referenced Dec 14, 2021
Closed
Closed
@leelavg leelavg requested a review from nbalacha December 14, 2021 06:56
nbalacha
nbalacha reviewed Dec 14, 2021
View reviewed changes
Copy link
Contributor

@nbalacha nbalacha left a comment

Choose a reason for hiding this comment

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

The commit msg for the first commit refers to changes made in commit2 . Please correct the commit msgs.

controllers/defaults.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Show resolved Hide resolved
config/rbac/topolvm_controller_role_bindings.yaml Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role_bindings.yaml Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role.yaml Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role.yaml Outdated Show resolved Hide resolved
config/rbac/topolvm_controller_role.yaml Outdated Show resolved Hide resolved
@leelavg leelavg requested a review from nbalacha December 16, 2021 10:53
sp98
sp98 suggested changes Dec 16, 2021
View reviewed changes
controllers/topolvm_controller.go Outdated Show resolved Hide resolved
controllers/topolvm_controller.go Show resolved Hide resolved
nbalacha
nbalacha suggested changes Dec 16, 2021
View reviewed changes
Copy link
Contributor

@nbalacha nbalacha left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I see this error when I run make deploy after make deploy.

...

Error from server (NotFound): error when deleting "STDIN": configmaps "manager-config" not found
Error from server (NotFound): error when deleting "STDIN": services "controller-manager-metrics-service" not found
Error from server (NotFound): error when deleting "STDIN": deployments.apps "controller-manager" not found

@leelavg
Copy link
Contributor Author

leelavg commented Dec 17, 2021

I run make deploy after make deploy.

  • I take that as make undeploy after make deploy
  • Did you see below towards the end when you run make deploy
configmap/manager-config created
service/controller-manager-metrics-service created
deployment.apps/controller-manager created
  • Pls confirm presence of above resources before you run undeploy that'd suffice
  • I tested atleast twice on OCP (3 master + 3 worker) and sometimes undeploy says deployment is not found cause namespace deletion triggers deployment deletion as well
  • And there's a chance that by the time above services are called for deletion by undeploy they were already deleted

@nbalacha
Copy link
Contributor

I run make deploy after make deploy.

  • I take that as make undeploy after make deploy
  • Did you see below towards the end when you run make deploy
configmap/manager-config created
service/controller-manager-metrics-service created
deployment.apps/controller-manager created
  • Pls confirm presence of above resources before you run undeploy that'd suffice
  • I tested atleast twice on OCP (3 master + 3 worker) and sometimes undeploy says deployment is not found cause namespace deletion triggers deployment deletion as well
  • And there's a chance that by the time above services are called for deletion by undeploy they were already deleted

I'll check but this error is not seen without this PR.

@leelavg
Copy link
Contributor Author

leelavg commented Dec 17, 2021

I'll check but this error is not seen without this PR.

  • ack, it's not seen in other PRs because there wasn't much roles/bindings/sa etc and above race will not happen
  • either ways, will await your confirmation

@leelavg
feat: implementation of topolvm controller resource unit
1671eba 
- implemented resourceManager interface of lvmcluster reconciler

Signed-off-by: Leela Venkaiah G <lgangava@redhat.com>
@leelavg
feat: added rbac for topolvm controller resource unit
2aa1dca 
- added operator rbac to deploy topolvm controller
- added CRD, controller rbac for topolvm controller to perform CSI functionalities

Signed-off-by: Leela Venkaiah G <lgangava@redhat.com>
@leelavg leelavg requested a review from nbalacha December 17, 2021 07:22
nbalacha
nbalacha suggested changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@nbalacha nbalacha left a comment

Choose a reason for hiding this comment

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

Sorry. Did not notice this in earlier reviews. Everything else looks fine though we should probably validate the rbacs later.

controllers/lvmcluster_controller_test.go Outdated Show resolved Hide resolved
controllers/lvmcluster_controller_test.go Outdated Show resolved Hide resolved
@leelavg
test: unit test for topolvm controller resource deployment
3cbe715 
Signed-off-by: Leela Venkaiah G <lgangava@redhat.com>
@leelavg
Copy link
Contributor Author

leelavg commented Dec 17, 2021

Sorry. Did not notice this in earlier reviews. Everything else looks fine though we should probably validate the rbacs later.

  • my mistake too in missing this, thanks for finding it.
  • validation of rbac wrt topolvm controller isn't needed, we should use consistent names for rbac used for lvm-operator after merging all components

@nbalacha
Copy link
Contributor

Sorry. Did not notice this in earlier reviews. Everything else looks fine though we should probably validate the rbacs later.

  • my mistake too in missing this, thanks for finding it.
  • validation of rbac wrt topolvm controller isn't needed, we should use consistent names for rbac used for lvm-operator after merging all components

@nbalacha nbalacha closed this Dec 17, 2021
@nbalacha nbalacha reopened this Dec 17, 2021
@nbalacha
Copy link
Contributor

Closed this by mistake. Apologies.

nbalacha
nbalacha approved these changes Dec 17, 2021
View reviewed changes
Copy link
Contributor

@nbalacha nbalacha left a comment

Choose a reason for hiding this comment

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

/lgtm

@jmolmo
Copy link
Contributor

jmolmo commented Dec 17, 2021

/lgtm

@nbalacha nbalacha merged commit dee0ba1 into openshift:main Dec 17, 2021
@leelavg leelavg deleted the csicontroller branch December 21, 2021 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbalacha nbalacha nbalacha approved these changes

@sp98 sp98 sp98 approved these changes

@jmolmo jmolmo jmolmo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@leelavg @nbalacha @jmolmo @sp98