-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add AgentPool support #279
base: master
Are you sure you want to change the base?
Conversation
Really cool to see this contribution for a highly demanded Azure resource, thank you for taking this on @vaspahomov! |
what is acutally missing to merge this PR? I would also like to have this functionnality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @vaspahomov for the PR. I have left some comments for discussion. Could you please also rebase your PR that will probably resolve the failing e2e-tests? Also could you please make sure all your commits are signed following the guide here. We also require them to be signed-off using something similar to git commit -s
(they already are signed-off btw). Thank you very much in advance.
if nodeCountNeedUpdate(c, az) { | ||
return true | ||
} | ||
if c.Spec.VnetSubnetID != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if spec.vnetSubnetID
is "" but az.VnetSubnetID != nil
, e.g., infrastructure object has been modified out of band? We need to be able to reconcile it back to the state specified by the spec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've marked vnetSubnetID
as immutable
} | ||
|
||
func nodeCountNeedUpdate(c *v1alpha3.AgentPool, az *containerservice.AgentPool) bool { | ||
if c.Spec.NodeCount != nil && c.Spec.MinNodeCount == nil && c.Spec.MaxNodeCount == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to update node count of the agent pool only if auto-scaling is disabled? And is auto-scaling disabled by setting min & max node counts to 0? If so, could you please explain this in-code with some comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've change conditions a bit here and add comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another aspect that we may consider is the late-initialization of AgentPool resources. Basically, it's initializing unset spec
fields with the defaults determined by Azure and obtained from the infrastructure object during Observe
calls. For a very concise example, please refer to:
https://github.com/crossplane/provider-azure/blob/33f728dbaadf9c9448d07ea16119d3137023d34d/pkg/controller/database/postgresqlserverconfiguration/managed.go#L140
There, we late-initialize the spec.forProvider.value
from what we observed in the corresponding azure-sdk-for-go
struct, i.e., in config.Value
in the above example.
9bfc3d4
to
d74cd2f
Compare
Signed-off-by: vaspahomov <vas2142553@gmail.com>
d74cd2f
to
435ad9f
Compare
Thanks @ulucinar I've commit fixes and ask some questions to your comments |
@ulucinar any chance to bring this forward? |
Currently, the managed resource API we define here does not conform to Crossplane's API patterns. For instance, we need to have the MR parameters under |
Doubling the latest comments: are there any chance to see it merged alongside the other PR @vaspahomov has opened? |
Hello!
This big patch with AgentPool resource support.
I can move AgentPool or/and AKSCluster in other api version of compute.
Also I can separate this patch into several.
Also noticed that it's impossible to remove system AgentPool from AKSCluster resource and change its props on AgentPoolRef. Azure can not create AKSCluster without system AgentPool. And azure ca not create AgentPool without ref on AKSCluster.
Description of your changes
Fixes #262 #277 #276
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Write some tests.
Manually tested. I've successfully deployed ManagedCluster and user AgentPool in azure via patched provider azure.