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

Initial scaffold for the official etcd-operator #6

Merged
merged 6 commits into from
Nov 1, 2024
Merged

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Oct 31, 2024

The exact steps:

  • Step 1: Initialize etcd-operator using kubebuilder
    The command is "kubebuilder init --domain etcd.io --repo go.etcd.io/etcd-operator"

    Note that the above command basically requires an empty directory, so run above command in anther empty directory, then move all generated files (without readme.md) & directories into etcd-operator.

  • Step 2: merge the readme.md into the existing file

  • Step 3: Create API
    The command is "kubebuilder create api --group clusters --version v1alpha1 --kind EtcdCluster".

    Answer "y" for both questions during the execution of above command.

  • Step 4: Edit EtcdClusterStatus to add two new fields

$ git diff
diff --git a/api/v1alpha1/etcdcluster_types.go b/api/v1alpha1/etcdcluster_types.go
index 9c2ac69..310f793 100644
--- a/api/v1alpha1/etcdcluster_types.go
+++ b/api/v1alpha1/etcdcluster_types.go
@@ -28,8 +28,10 @@ type EtcdClusterSpec struct {
        // INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
        // Important: Run "make" to regenerate code after modifying this file
 
-       // Foo is an example field of EtcdCluster. Edit etcdcluster_types.go to remove/update
-       Foo string `json:"foo,omitempty"`
+       // Size is the expected size of the etcd cluster.
+       Size int `json:"size"`
+       // Version is the expected version of the etcd container image.
+       Version string `json:"version"`
 }
 
 // EtcdClusterStatus defines the observed state of EtcdCluster.
  • Step 5: Generate manfiest
    The command is "make manifests"

  • Step 6: Verification
    Verified in kubernetes cluster 1.31.2. The commands:

$ make install
$ make run

Note that all details are also included in each commit messages.
cc @hakman @jberkus @jmhbnz @justinsb @neolit123

Please note this is just a very first base on which we can continue to work on.
We also need more contributors to help us continue working on v0.1.0. Thank you!

The command is:
kubebuilder init --domain etcd.io --repo go.etcd.io/etcd-operator

But the generated readme.md file is removed for now, and it will be
merged to the existing readme.md in next commit.

Signed-off-by: Benjamin Wang <[email protected]>
The command is:
kubebuilder create api --group clusters --version v1alpha1 --kind EtcdCluster

and input two "y"

Signed-off-by: Benjamin Wang <[email protected]>
The command is:
make manifests

Signed-off-by: Benjamin Wang <[email protected]>
@ahrtr
Copy link
Member Author

ahrtr commented Oct 31, 2024

Removed the lint workflow for now.

name: Lint

on:
  push:
  pull_request:

jobs:
  lint:
    name: Run on Ubuntu
    runs-on: ubuntu-latest
    steps:
      - name: Clone the code
        uses: actions/checkout@v4

      - name: Setup Go
        uses: actions/setup-go@v5
        with:
          go-version: '~1.22'

      - name: Run linter
        uses: golangci/golangci-lint-action@v6
        with:
          version: v1.59

@jberkus
Copy link
Contributor

jberkus commented Oct 31, 2024

Have we always run our tests on ubuntu? Most of Kubernetes uses Debian.

@ahrtr
Copy link
Member Author

ahrtr commented Oct 31, 2024

Have we always run our tests on ubuntu? Most of Kubernetes uses Debian.

This is just the very first scaffold. We can continue to iterate based on this.

@ahrtr
Copy link
Member Author

ahrtr commented Oct 31, 2024

To be clearer, almost all source code and manifests are automatically generated by kubebuilder. I only manually added two new fields in EtcdClusterStatus at step 4.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Great to get the initial scaffold checked in thanks @ahrtr

@@ -0,0 +1,25 @@
{
"name": "Kubebuilder DevContainer",
"image": "golang:1.22",
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to align this with main etcd-io/etcd devcontainer. Happy to address it in follow-up.

Suggested change
"image": "golang:1.22",
"image": "mcr.microsoft.com/devcontainers/go:1.23-bookworm"

@@ -0,0 +1,35 @@
name: E2E Tests
Copy link
Member

Choose a reason for hiding this comment

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

Let's migrate these to prow in a follow-up.

@@ -0,0 +1,33 @@
# Build the manager binary
FROM golang:1.22 AS builder
Copy link
Member

Choose a reason for hiding this comment

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

Move to 1.23 in a follow-up.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, hakman, jmhbnz

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 [ahrtr,hakman,jmhbnz]

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

@ahrtr
Copy link
Member Author

ahrtr commented Nov 1, 2024

Thanks all for the review. We can address the comments in followup PRs.

@ahrtr ahrtr merged commit ccbf648 into main Nov 1, 2024
5 checks passed
Comment on lines +31 to +34
// Size is the expected size of the etcd cluster.
Size int `json:"size"`
// Version is the expected version of the etcd container image.
Version string `json:"version"`

Choose a reason for hiding this comment

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

i suggest the following API for alpha1 so that at minimum the user has the flexibility to set the image from airgap registries.

// Size is the expected size of the etcd cluster.
Size int
// Image is the path:tag or path@digest of a container image to use for the etcd cluster members. 
Image string

this area of the API later can be extended to the following:

type Member struct {
  Name string (omitifempty, should always be prefixed with the cluster name, generated to member1,2,3 if not specififed)
  Image string (omitifempty, overrides defaults)
}

type EtcdClusterSpec struct {
  Name string (name of the cluster)
  DefaultImage string
  Members []Members
}

Copy link
Member Author

Choose a reason for hiding this comment

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

i suggest the following API for alpha1 so that at minimum the user has the flexibility to set the image from airgap registries.

// Size is the expected size of the etcd cluster.
Size int
// Image is the path:tag or path@digest of a container image to use for the etcd cluster members. 
Image string

I understand the motivation of supporting airgap use case.

My original design doc had one more field "registry" to support such case. But removed it to keep v0.1.0 as simple as possible. It's open to add it back or follow your proposal in next step.

There is also a concern in your proposal, using a digest might be difficult for the operator to compare the version (old version vs new version) when upgrading. We don't support upgrade the etcd cluster across two minor versions, also do not support downgrading for now. So we need to compare the version.

this area of the API later can be extended to the following:

type Member struct {
  Name string (omitifempty, should always be prefixed with the cluster name, generated to member1,2,3 if not specififed)
  Image string (omitifempty, overrides defaults)
}

type EtcdClusterSpec struct {
  Name string (name of the cluster)
  DefaultImage string
  Members []Members
}

I am not sure whether I fully understood your point. Each time when users scale in/out the cluster, users need to update the member list themselves? It seems that it has more flexibility but also a little more complicated. The current design is that users only need to specify a size.

It's open to discussion for next release, i.e. 0.2.0+.

Could you please raise issues to track your proposals? Thanks

Choose a reason for hiding this comment

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

There is also a concern in your proposal, using a digest might be difficult for the operator to compare the version (old version vs new version) when upgrading. We don't support upgrade the etcd cluster across two minor versions, also do not support downgrading for now. So we need to compare the version.

the target cluster and it's members could have a new digest.
that would allow upgrading from one digest -> another.
digest is considered more secure that tag but that argument is not a good one.
so supporting only tags is fine in my book.

I am not sure whether I fully understood your point. Each time when users scale in/out the cluster, users need to update the member list themselves? It seems that it has more flexibility but also a little more complicated. The current design is that users only need to specify a size.

it's just an idea - to have a current cluster state and a target cluster state.
the target cluster can have a different amount of members e.g. scale to 5, but also making it possible to override the image per member if this operator can be used to verify etcd upgrade or skew.

Could you please raise issues to track your proposals? Thanks

i think before raising issues we should spend the zoom calls and do API planning / review.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think before raising issues we should spend the zoom calls and do API planning / review.

Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

do API planning / review.

@neolit123 would you be able to help draft an API design/plan when you have time?

Choose a reason for hiding this comment

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

sure, i will draft something for the alpha.

@ahrtr ahrtr deleted the apis_20241031 branch November 1, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants