✨ Feat/allow skipping ami in launch template when EKS managed nodegroup #4388
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
Welcome @Julian-Chu! |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @Julian-Chu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pkg/cloud/services/ec2/service.go
Outdated
| @@ -32,6 +32,8 @@ type Service struct { | |||
|
|
|||
| // SSMClient is used to look up the official EKS AMI ID | |||
| SSMClient ssmiface.SSMAPI | |||
| // isMock is used to return current mock service instead of creating new one, for testing | |||
| isMock bool | |||
There was a problem hiding this comment.
You don't need this. Could you take a look at other tests which create this service? :) You can see that we are mocking the scope, which is then, in turn, used to set up these services.
There was a problem hiding this comment.
hey @Skarlso
thanks for comment. I saw servicefactory pattern in controller like
// https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/exp/controllers/awsmachinepool_controller.go#L74
func (r *AWSMachinePoolReconciler) getEC2Service(scope scope.EC2Scope) services.EC2Interface {
if r.ec2ServiceFactory != nil {
return r.ec2ServiceFactory(scope)
}
return ec2.NewService(scope)
}the issue here is that EC2Service has to create new EC2Service inside the ReconcileLaunchTemplate method, so I can't apply serviceFactory to it. Can we reuse EC2Service instead of creating new one? Any suggestion? 🙏
// https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/pkg/cloud/services/ec2/launchtemplate.go#L64
func (s *Service) ReconcileLaunchTemplate(
scope scope.LaunchTemplateScope,
canUpdateLaunchTemplate func() (bool, error),
runPostLaunchTemplateUpdateOperation func() error,
) error {
...
ec2svc := NewService(scope.GetEC2Scope())
...
}
type Service struct {
scope scope.EC2Scope
EC2Client ec2iface.EC2API
// SSMClient is used to look up the official EKS AMI ID
SSMClient ssmiface.SSMAPI
}
// NewService returns a new service given the ec2 api client.
func NewService(clusterScope scope.EC2Scope) *Service {
return &Service{
scope: clusterScope,
EC2Client: scope.NewEC2Client(clusterScope, clusterScope, clusterScope, clusterScope.InfraCluster()),
SSMClient: scope.NewSSMClient(clusterScope, clusterScope, clusterScope, clusterScope.InfraCluster()),
}
}There was a problem hiding this comment.
another idea is to create factory in EC2Service, but we still need new fields.
func (s *Service) new(clusterScope scope.EC2Scope) *Service {
var ec2Client ec2iface.EC2API
if s.ec2ClientFactory != nil {
ec2Client = s.ec2ClientFactory()
} else {
ec2Client = scope.NewEC2Client(clusterScope, clusterScope, clusterScope, clusterScope.InfraCluster())
}
var ssmClient ssmiface.SSMAPI
if s.ssmClientFactory != nil {
ssmClient = s.ssmClientFactory()
} else {
ssmClient = scope.NewSSMClient(clusterScope, clusterScope, clusterScope, clusterScope.InfraCluster())
}
return &Service{
scope: clusterScope,
EC2Client: ec2Client,
SSMClient: ssmClient,
}
}There was a problem hiding this comment.
I'm so sorry, I will reply to this either this night or tomorrow morning.
@Skarlso PING
There was a problem hiding this comment.
Okay, so you don't need any of that because you have the scope and the ec2Mock EXPECTs. And the scope is mocked and you can control the ec2 service responses from there. From example, GetEC2Scope will return InfraCluster from the MachinePool scope.
So when you set things up here:
InfraCluster: mcps,you should be able to return something canned. I can take a look at that later if you don't find anything. :)
There was a problem hiding this comment.
Ah crap, I see what you mean. And I see now that we do that with the Service Factory pattern:
ec2ServiceFactory: func(scope.EC2Scope) services.EC2Interface {
return ec2Svc
},Okay. Would you mind please switching this isMock to that then please? Cheers!
There was a problem hiding this comment.
Thanks, and sorry it took so long to reply. Eventually, I'll rework this into using proper injection pattern. But that's not gonna happen in this PR. :) Cheers.
There was a problem hiding this comment.
hi @Skarlso ,
sorry for late reply, I refactored it in refactor to service factory pattern. If you have time, please take a look.
if you have already done rework in another PR, I would be happy to follow that. 😄
|
/retest |
1 similar comment
|
/retest |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@Julian-Chu do you require any help here to proceed with the PR? |
thanks @Ankitasw . got new comments, I will push new commit when available. |
|
/retest |
|
/milestone v2.4.0 |
|
/retest |
Julian-Chu
force-pushed
the
feat/allow-skip-ami-launch-template
branch
from
06662fd to
d09d6e8
Compare
|
/retest |
|
/retest |
|
hi @Ankitasw, may I have your help? regarding to this error message in PR verify step |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/milestone clear |
Ankitasw
changed the title
Sorry for late reply, added the PR type for you. |
|
@Julian-Chu: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
EKS managed node group can have empty AMI id in launch template. It allows EKS manage AMI for us with custom launch template like user data or storage
Which issue(s) this PR fixes :
Fixes #4377
Special notes for your reviewer:
Checklist:
Release note: