Refactor KubemarkCluster into ComputeCluster #66

fabriziopandini · 2022-12-22T14:04:09Z

What this PR does / why we need it:
Refactor the current KubemarkCluster into a compute cluster; this is required for two reasons:

To disambiguate from the KubemarkCluster" CRD that will be introduced in the following steps
To ensure this component does not become a bottleneck at scale (it currently creates a client at each reconciliation, and we already saw in Cluster API that this is a problem; it must be refactored by implementing something similar to the cluster tracker in CAPI)

Which issue this PR fixes:
rif: #63

Special notes for your reviewer:

Release notes:

Deprecate Machine.Spec.KubemarkHollowPodClusterSecretRef in favor of Machine.Spec.BackingCluster

k8s-ci-robot · 2022-12-22T14:04:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

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

fabriziopandini · 2022-12-22T14:04:39Z

/hold
for #64 and #65 to merge first

elmiko

just giving a cursory review of the code, the BackingCluster name caught my eye

elmiko · 2022-12-22T16:50:31Z

api/v1alpha4/kubemarkmachine_types.go

+	// in case also KubemarkCluster.spec.backingCluster is empty or if the cluster uses a different infrastructure cluster kind like
+	// e.g. DockerCluster, pods running kubemark will be created in the management cluster, in the same namespace of the cluster resource.
+	// +optional
+	BackingCluster *BackingClusterSpec `json:"backingCluster,omitempty"`


BackingCluster sounds a little ambiguous to me, maybe we could call it KubemarkPodHostCluster ?

@elmiko what about ComputeCluster?

looking at the old value that this new approach will deprecate, we named it KubemarkHollowPodClusterSecretRef, what if we call this new field KubemarkPodCluster ?

i just don't want users to get confused when they see this, i feel like we have a couple references to different "clusters" and the topology can become confusing.

I have the same concern, but when I looked at the old name my reaction was that this is exposing a lot of internals the user should not really care about (the fact that we run a binary that is called kubemark, the fact that this thing runs a Pod etc).

It is also limiting because we are not creating only pods, but also secrets, and potentially more in the future, and (nit) having a field in a Kubemark* object that repeats kubemark kind of stutter

This is is why I would prefer something that is easier to explain

this is where the backend of my machines runs (BackingCluster)

this is the thing that provides computing power to my machines (ComputeCluster)

(???)

But I leave to you the final call, just tell me what do you pick

i like your reasoning, i'm happy to go forward with ComputeCluster. thanks for explaining =)

fabriziopandini · 2023-01-04T17:48:59Z

/hold cancel
PR should be ready (pending discussion about naming)

elmiko

i was able to test this using a kind cluster and the docker control plane method of deployment. when i used the ComputeCluster field i could see kubemark pods spawning on the target cluster.

i did hit one snag that i think we should document before merging, i left a comment inline

api/v1alpha4/compute_cluster.go

fabriziopandini · 2023-01-14T11:06:16Z

@elmiko this one should be ready, but let's keep it on hold till we get e2e tests running by merging #69 + corresponding test infra PRs (TODO)
/hold

k8s-ci-robot · 2023-01-16T18:38:32Z

@fabriziopandini: 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.

k8s-triage-robot · 2023-04-16T19:08:25Z

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

k8s-triage-robot · 2023-05-16T19:26:08Z

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle rotten

elmiko · 2023-05-17T14:10:01Z

the upstream work in cluster-api is still on-going, i'm happy to keep this pull open.

/remove-lifecycle rotten

k8s-triage-robot · 2024-01-19T05:57:04Z

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

k8s-triage-robot · 2024-02-18T06:04:14Z

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle rotten

k8s-triage-robot · 2024-03-19T06:46:00Z

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:

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 PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2024-03-19T06:46:04Z

@k8s-triage-robot: Closed this PR.

In response to this:

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:

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 PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 22, 2022

k8s-ci-robot requested review from CecileRobertMichon and detiber December 22, 2022 14:04

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 22, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2022

elmiko reviewed Dec 22, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2022

fabriziopandini force-pushed the refactor-kubemark-cluster-into-backing-cluster branch from f9b4e8e to b758b2c Compare December 27, 2022 15:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2022

fabriziopandini changed the title ~~[WIP] Refactor KubemarkCluster into BackingCluster~~ Refactor KubemarkCluster into ComputeCluster Dec 27, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 27, 2022

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 4, 2023

elmiko reviewed Jan 10, 2023

View reviewed changes

api/v1alpha4/compute_cluster.go Show resolved Hide resolved

refactor KubemarkCluster into ComputeCluster

c2f5cb3

fabriziopandini force-pushed the refactor-kubemark-cluster-into-backing-cluster branch from b758b2c to c2f5cb3 Compare January 14, 2023 11:03

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2023

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 14, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 16, 2023

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 16, 2023

k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 16, 2023

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 17, 2023

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 19, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 18, 2024

k8s-ci-robot closed this Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor KubemarkCluster into ComputeCluster #66

Refactor KubemarkCluster into ComputeCluster #66

fabriziopandini commented Dec 22, 2022 •

edited

k8s-ci-robot commented Dec 22, 2022

fabriziopandini commented Dec 22, 2022

elmiko left a comment

elmiko Dec 22, 2022

fabriziopandini Dec 23, 2022

elmiko Jan 3, 2023

fabriziopandini Jan 4, 2023

elmiko Jan 4, 2023

fabriziopandini commented Jan 4, 2023

elmiko left a comment

fabriziopandini commented Jan 14, 2023

k8s-ci-robot commented Jan 16, 2023

k8s-triage-robot commented Apr 16, 2023

k8s-triage-robot commented May 16, 2023

elmiko commented May 17, 2023

k8s-triage-robot commented Jan 19, 2024

k8s-triage-robot commented Feb 18, 2024

k8s-triage-robot commented Mar 19, 2024

k8s-ci-robot commented Mar 19, 2024

Refactor KubemarkCluster into ComputeCluster #66

Refactor KubemarkCluster into ComputeCluster #66

Conversation

fabriziopandini commented Dec 22, 2022 • edited

k8s-ci-robot commented Dec 22, 2022

fabriziopandini commented Dec 22, 2022

elmiko left a comment

Choose a reason for hiding this comment

elmiko Dec 22, 2022

Choose a reason for hiding this comment

fabriziopandini Dec 23, 2022

Choose a reason for hiding this comment

elmiko Jan 3, 2023

Choose a reason for hiding this comment

fabriziopandini Jan 4, 2023

Choose a reason for hiding this comment

elmiko Jan 4, 2023

Choose a reason for hiding this comment

fabriziopandini commented Jan 4, 2023

elmiko left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jan 14, 2023

k8s-ci-robot commented Jan 16, 2023

k8s-triage-robot commented Apr 16, 2023

k8s-triage-robot commented May 16, 2023

elmiko commented May 17, 2023

k8s-triage-robot commented Jan 19, 2024

k8s-triage-robot commented Feb 18, 2024

k8s-triage-robot commented Mar 19, 2024

k8s-ci-robot commented Mar 19, 2024

fabriziopandini commented Dec 22, 2022 •

edited