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

EKS kubelet-extra-args are not set when using managedmachinepool #2224

Open
felipeweb opened this issue Jan 23, 2021 · 22 comments
Open

EKS kubelet-extra-args are not set when using managedmachinepool #2224

felipeweb opened this issue Jan 23, 2021 · 22 comments
Assignees
@richardcase
Labels
area/provider/eks Issues or PRs related to Amazon EKS provider help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@felipeweb
Copy link
Contributor

felipeweb commented Jan 23, 2021 •
edited

/kind bug

What steps did you take and what happened:

apiVersion: exp.cluster.x-k8s.io/v1alpha3
kind: MachinePool
metadata:
  name: undistro-quickstart-mp-0
  namespace: default
spec:
  clusterName: undistro-quickstart
  minReadySeconds: 0
  replicas: 1
  template:
    metadata: {}
    spec:
      bootstrap:
        configRef:
          apiVersion: bootstrap.cluster.x-k8s.io/v1alpha3
          kind: EKSConfig
          name: undistro-quickstart-mp-033dcec9-adbf-4e6d-b493-6db22e2d0e30-0
          namespace: default
      clusterName: undistro-quickstart
      infrastructureRef:
        apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
        kind: AWSManagedMachinePool
        name: undistro-quickstart-mp-0
        namespace: default
      version: v1.18.9
---
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSManagedMachinePool
metadata:
  name: undistro-quickstart-mp-0
  namespace: default
spec:
  additionalTags:
    infra-node: "true"
  amiType: AL2_x86_64
  eksNodegroupName: undistro-quickstart-mp-0
  instanceType: t3.medium
  roleName: nodes.cluster-api-provider-aws.sigs.k8s.io
  scaling:
    maxSize: 5
    minSize: 1
---
apiVersion: bootstrap.cluster.x-k8s.io/v1alpha3
kind: EKSConfig
metadata:
  name: undistro-quickstart-mp-033dcec9-adbf-4e6d-b493-6db22e2d0e30-0
  namespace: default:
spec:
  kubeletExtraArgs:
    node-labels: node-role.undistro.io/infra=true
    register-with-taints: dedicated=infra:NoSchedule

is using wrong userdata

/etc/eks/bootstrap.sh default_undistro-quickstart --kubelet-extra-args '--node-labels=eks.amazonaws.com/nodegroup-image=ami-0dd0589ee7a07c236,eks.amazonaws.com/capacityType=ON_DEMAND,eks.amazonaws.com/nodegroup=undistro-quickstart-mp-0' --b64-cluster-ca $B64_CLUSTER_CA --apiserver-endpoint $API_SERVER_URL --dns-cluster-ip $K8S_CLUSTER_DNS_IP

What did you expect to happen:

/etc/eks/bootstrap.sh default_undistro-quickstart --kubelet-extra-args '--node-labels=node-role.undistro.io/infra=true,eks.amazonaws.com/nodegroup-image=ami-0dd0589ee7a07c236,eks.amazonaws.com/capacityType=ON_DEMAND,eks.amazonaws.com/nodegroup=undistro-quickstart-mp-0 --register-with-taints=dedicated=infra:NoSchedule' --b64-cluster-ca $B64_CLUSTER_CA --apiserver-endpoint $API_SERVER_URL --dns-cluster-ip $K8S_CLUSTER_DNS_IP

Anything else you would like to add:

Environment:

  • Cluster-api-provider-aws version: v0.6.4
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 23, 2021
@felipeweb felipeweb changed the title EkS kubelet-extra-args are not set when using machinepool EKS kubelet-extra-args are not set when using machinepool Jan 23, 2021
@richardcase
Copy link
Member

/area provider/eks
/help

@k8s-ci-robot
Copy link
Contributor

@richardcase:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/area provider/eks
/help

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 k8s-ci-robot added area/provider/eks Issues or PRs related to Amazon EKS provider help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 23, 2021
@felipeweb felipeweb mentioned this issue Jan 23, 2021
Closed
@felipeweb
Copy link
Contributor Author

felipeweb commented Jan 23, 2021 •
edited

@richardcase the problem is that the machinepool is not converting template object into a concrete object. I sent a PR to CAPI, but it seems to do not work with concrete objects too. I will make a more deep investigation

@felipeweb
Copy link
Contributor Author

felipeweb commented Jan 24, 2021 •
edited

We are using the default launch template for eks, I am assuming this because we are not passing any launch template

cluster-api-provider-aws/pkg/cloud/services/eks/nodegroup.go

Lines 162 to 171 in b7294ce

input := &eks.CreateNodegroupInput{
ScalingConfig: s.scalingConfig(),
ClusterName: aws.String(eksClusterName),
NodegroupName: aws.String(nodegroupName),
Subnets: aws.StringSlice(s.subnets()),
NodeRole: roleArn,
Labels: aws.StringMap(managedPool.Labels),
Tags: aws.StringMap(tags),
RemoteAccess: remoteAccess,
}

There are any reasons for that?

According to AWS Console the userdata is configured into launch template so we need to reconcile and pass a launch template in node group creation.

@richardcase could you give me the basic paths about where start to fix this? I'm newbie into capa codebase.

I am assuming the starting point is here

cluster-api-provider-aws/exp/controllers/awsmanagedmachinepool_controller.go

Line 190 in 634f30e

if err := ekssvc.ReconcilePool(); err != nil {

@felipeweb felipeweb changed the title EKS kubelet-extra-args are not set when using machinepool EKS kubelet-extra-args are not set when using managedmachinepool Jan 24, 2021
@richardcase
Copy link
Member

@felipeweb - there isn't a specific reason the launch template support isn't there for AWSManagedMachinePool....it was really to do with timing and us wanting to get initial functionality into the provider (which meant using the default launch template). We have an issue created for adding the launch template support ( #2055)....... @michaelbeaumont are you working on this or is this something @felipeweb can help with?

There is launch template support on the non-managed machine pool (AWSMachinePool) that may be worth looking at......and potentially refactoring so that it can be reused.

@richardcase
Copy link
Member

@felipeweb - also feel free to ping me on the slack channel if needed

@michaelbeaumont
Copy link
Contributor

michaelbeaumont commented Jan 24, 2021 •
edited

@richardcase I am working on it, I'll have a PR up today. We can gladly collaborate on it! @felipeweb

@richardcase
Copy link
Member

Great @michaelbeaumont 👍

@michaelbeaumont michaelbeaumont mentioned this issue Jan 24, 2021
Closed
2 tasks
@michaelbeaumont
Copy link
Contributor

I'm not yet convinced EKSConfig should be supported at all. (some more discussion here)

If we let the AWSManagedMachinePool spec look like the EKS API then we have to support both spec.labels as well as labels set through EKSConfig. In order to support EKSConfig, we have to launch everything through launch templates and setup user data ourselves, invalidating spec.labels.

The API to managed nodegroups provides a lot of the things that users would set through kubeletExtraArgs. Should we support both and assume the user knows what they're doing? Or do we take the opposite approach and not even expose the simpler managed nodegroup API interface?

@richardcase @felipeweb

@felipeweb
Copy link
Contributor Author

@michaelbeaumont @richardcase I understand the risks of supporting both formats, but even so, I think we must because the kubelet-extra-args can configure the taints in addition to the labels

@richardcase
Copy link
Member

If we support both and there is overlap which would take precedence?

(I need to have a look at the docs to give a better answer....will do that)

@michaelbeaumont
Copy link
Contributor

Indeed, there are certain features that managed nodegroups don't yet natively support.

This all make me wonder if it's worth supporting the structure of the managed nodegroup API in the AWSManagedMachinePool spec at all (i.e. how it is currently).

What do we think about have some common part of spec for AWSMachinePool and AWSManagedMachinePool such that they both support arbitrary customization? Right now there is a launchTemplate field for AWSMachinePool but it doesn't actually support customizing the launch template very much beyond a few parameters, in that sense rather similar to the current AWSManagedMachinePool spec.
Do we want users to be able to supply an arbitrary launch template?

Ultimately, what do we actually want these specs to look like?

@richardcase
Copy link
Member

/milestone v0.7.x

@k8s-ci-robot k8s-ci-robot added this to the v0.7.x milestone Mar 12, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 10, 2021
@richardcase
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 10, 2021
@richardcase
Copy link
Member

/assign
/lifecycle active

@k8s-ci-robot k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Sep 23, 2021
@richardcase richardcase mentioned this issue Sep 30, 2021
Closed
@randomvariable randomvariable modified the milestones: v0.7.x, v1.x, v1.2.0 Nov 8, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels Feb 6, 2022
@richardcase
Copy link
Member

This is going to be fixed by #3094

@richardcase
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 7, 2022
@richardcase
Copy link
Member

/remove-lifecycle frozen

@k8s-ci-robot k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jul 12, 2022
@richardcase
Copy link
Member

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.5.0 milestone Jul 25, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@richardcase richardcase

Labels
area/provider/eks Issues or PRs related to Amazon EKS provider help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Milestone
No milestone
7 participants
@richardcase @randomvariable @michaelbeaumont @felipeweb @k8s-ci-robot @fejta-bot @k8s-triage-robot