-
Notifications
You must be signed in to change notification settings - Fork 38
NetworkCompartmentId implementation + fixes for OCIManagedMachinePool #455
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
base: main
Are you sure you want to change the base?
Conversation
What happens if a user 1) updates the network compartment ID to a new compartment or 2) deletes the network compartment id? |
Instead of calling out all the places tests need to be added I'll just make the blanket statement here. We need to make sure we have some unit test coverage for this update. I know we talked about adding e2e tests around this already as well, but I'm just putting this in the comment for posterity. |
I will need to think how to handle the networkCompartmentId update/deletion. |
We talked about this and we came to the conclusion that update/delete of the compartmentID won't work as it is setup now. We won't address this as part of this PR. At somepoint in the future we will induce this but it will need to leverage https://docs.oracle.com/en-us/iaas/api/#/en/iaas/latest/Vcn/ChangeVcnCompartment. This will be a bigger lift than just adding it as part of this PR. |
What this PR does / why we need it:
This PR introduces a new field for OCIManagedCluster that allows for the network to be created in a different compartment than the one specified in
OCIManagedCluster.Spec.CompartmentId
The new field is called
OCIManagedCluster.Spec.NetworkSpec.CompartmentId
, it is optional and if not set it will default to the only compartment specified.In addition, this PR fixes many bugs on the managed machine pool controller. The controller logic caused continues updates on the node pool if the original OCIManagedCluster was modified. This is because not all fields were set as the actual managed node pool state and updated correctly.
The new reconciliation logic is following this design:
Which issue(s) this PR fixes:
Fixes #450