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.

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 AWSFargateProfile ADR #2250

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

michaelbeaumont
Copy link
Contributor

@michaelbeaumont michaelbeaumont commented Feb 15, 2021

What this PR does / why we need it:
Documents a major decision around EKS Fargate support, whether they map to a MachinePool.

Which issue(s) this PR fixes:
Part of #1786 (#1786 (comment))

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

New ADR to document the decision of how Fargate Profiles will be represented.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 15, 2021
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 15, 2021
@richardcase
Copy link
Member

/kind documentation

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Feb 16, 2021
Copy link
Member

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Looks good @michaelbeaumont . A few minor comments...

docs/adr/0004-fargate-pools.md Outdated Show resolved Hide resolved
docs/adr/0004-fargate-pools.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 16, 2021
@michaelbeaumont michaelbeaumont changed the title 📖 Add FargatePool ADR 📖 Add AWSFargatePool ADR Feb 16, 2021
@richardcase
Copy link
Member

This seems good to me. Lets get a few others to review and get some consensus...until then:

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2021

## Context

At the time of writing CAPA has the AWS-specific infrastructure resources `AWSMachinePool`s
Copy link
Member

Choose a reason for hiding this comment

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

If we are not mapping to a MachinePool then shall we remove 'Pool' from the kind name and go with something like AWSFargateProfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. I like the word "pool" better in general but it does have the MachinePool connotation. Profile is kind of meaningless IMO, but since that's what the AWS object is called, it wouldn't be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah i see your point....'profile' is a bit meaningless. It is a 'pool' of compute resource to use but it sort of implies MachinePool to me. But if machinepools are changing in v1alpha4 then perhaps it would be better to keep it as is?

Choose a reason for hiding this comment

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

I think keeping the "Pool" name might get confusing, especially if there is no mapping with MachinePools. MachinePools might change but they will still have different objectives and there is no plan to change the name. See kubernetes-sigs/cluster-api#4063 for details.

Copy link
Member

Choose a reason for hiding this comment

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

@michaelbeaumont - what do you think could be a good alternative to using Pool or Profile? Just some ideas: AWSFargateRule, AWSFargatePodExecution, AWSFargateExecutionConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's go with the original suggestion, AWSFargateProfile

@randomvariable
Copy link
Member

I wonder if @CecileRobertMichon and @devigned have any ideas on related concepts in CAPZ and proposed changes for MachinePools overall.

@richardcase
Copy link
Member

I wonder if @CecileRobertMichon and @devigned have any ideas on related concepts in CAPZ and proposed changes for MachinePools overall.

I wonder if using Azure Container Instances with AKS is similar? ....don't know enough about ACI to answer that.

@devigned
Copy link

Wow, this is timely. @CecileRobertMichon and I have been prototyping a similar idea on Azure Container Instances (https://github.com/kubernetes-sigs/cluster-api-provider-azure/compare/master...devigned:aci?expand=1). There is no proposal yet, but you can see the direction via the diff above.

The way we are currently approaching the problems is by running virtual kubelet as a container on ACI to then act as a virtual node in the cluster with infinite resources. We have a AzureContainerInstanceMachine which acts like any normal workload machine, bootstrap + infrastructure reference. We are mounting the kubeadm bootstrap data into the container, running cloud-init in the container 🤮, and joining the virtual kubelet to the cluster. This is not totally working yet, but super close.

I had thought about using the machine pool abstraction, but the ideas didn't align in my mind. The abstraction for machine pool is more focused on running machines, VMs, not containers. If we could start a node image image on ACI or Fargate, like Kind does, then run docker in docker, then I think this would be the right abstraction. Unfortunately, for ACI, I don't know about Fargate, we are unable to run privileged workloads. That pretty much kills the docker in docker scenario, afaik.

We should totally discuss serverless CAPI 🚀

@michaelbeaumont
Copy link
Contributor Author

It sounds like Fargate operates at a higher abstraction level in EKS than ACI in AKS. Fargate profiles are basically just lists of pod label and namespace selectors which match pods with a certain annotation and then run these pods in their own environment. In fact, every pod running in a Fargate profile appears as a Kubernetes node.
There's no clear link between nodes and their profile though, but a replica count wouldn't be especially meaningful anyway.

@richardcase
Copy link
Member

@michaelbeaumont - i would agree that it operates on a higher level of abstraction. But the commonality here i think is that ACI and Fargate don't match the machine pool. Based on this i think what is proposed in the ADR is good.

@CecileRobertMichon
Copy link

It seems like the goal here is slightly different than what @devigned and I are prototyping. This seems to be about enabling Fargate-backed virtual nodes on EKS (the equivalent of the AKS virtual node addon). We're looking at implementing an ACI infra provider for machines (similar to CAPD machines, but in ACI).

@richardcase
Copy link
Member

This has been decided now, so:

/unhold
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 38007ac into kubernetes-sigs:master Feb 18, 2021
@michaelbeaumont michaelbeaumont deleted the fargate_adr branch February 18, 2021 10:41
@michaelbeaumont michaelbeaumont changed the title 📖 Add AWSFargatePool ADR 📖 Add AWSFargateProfile ADR Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants