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 implementation for embedded datastore #500

Merged
merged 40 commits into from
Jul 3, 2024
Merged

Conversation

neoaggelos
Copy link
Contributor

@neoaggelos neoaggelos commented Jun 17, 2024

Summary

Initial implementation for etcd datastore option.

Requires canonical/k8s-dqlite#105

Changes

  • Add pkg/client/etcd to interact with the embedded cluster
  • Add new etcd datastore type
  • Add new bootstrap configuration options to optionally seed k8s-snap with external certificates for the etcd cluster

Notes for reviewers

  • Start by reviewing the accompanying docs, as they tell most of the story.

@neoaggelos neoaggelos requested a review from a team as a code owner June 17, 2024 06:55
@@ -0,0 +1,138 @@
# How to use the embedded datastore

Canonical Kubernetes supports using an embedded datastore such as etcd
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could lead to some level of confusion. It confused me. Mainly because this implies that there may be other options for an 'embedded' datastore (and slightly because it is etcd which may be slightly confusing as thats the example we use for an external datastore too).
From what I can see this means that we are building in our own etcd alongside dqlite and either can be used at deploy-time. It isn't entirely clear what each will be used for.
So in short, I think we need a much more detailed explanation document which covers the datastore, which we can then refer to from here and keep the explanations where they need to be, freeing this up to be a more straightforward howto

Copy link
Member

Choose a reason for hiding this comment

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

Indeed embedded seems unclear to end users. dqlite is also positioned as an embedded database so if the user selects embedded as a datastore does it mean he wants dqlite or etcd? I think the intention here is to let users slect the option of a "managed-etcd". With "managed-etcd" it is clear you do not want "managed-dqlite" or "external-etcd".

Also the word "embedded" leads you to believe that etcd is embedded into k8s whereas in reality we keep it in its own service. The end user probably does not know that the correct term used among engineers doing the implementation of that that service is that they use an embedded etcd.

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Great work (most questions resolved offline).

As a general note: why do we call this embedded? At least I found it a bit confusing as it does not really convey that etcd is used. Can we not just call it etcd (at least for user facing configs)? Then we have datastore-type: k8s-dqlite|etcd|external which would make sense to me.

build-scripts/components/k8s-dqlite/version Outdated Show resolved Hide resolved
docs/src/snap/howto/embedded-datastore.md Outdated Show resolved Hide resolved
docs/src/snap/howto/embedded-datastore.md Outdated Show resolved Hide resolved

// NOTE(neoaggelos): update kube-apiserver arguments in case cluster nodes have changed, but do not
// restart kube-apiserver, to avoid downtime for existing cluster nodes. The next time kube-apiserver
// restarts on this node, they will use the updated arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds dangerous. What if the kube-apiserver just don't restart for a long time. This would let the file arguments and what is configured on the running instance diverge/run out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would diverge but not run out of sync. The local etcd node will always be part of the etcd cluster, as long as it remains a control plane node.


func NewEtcdPKI(opts EtcdPKIOpts) *EtcdPKI {
if opts.Years == 0 {
opts.Years = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when the self-signed CA expires? Do you have to manually create new ones?

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Great work, some minor nits then it is good to ship.

docs/src/snap/explanation/datastore/etcd.md Outdated Show resolved Hide resolved
docs/src/snap/explanation/datastore/external.md Outdated Show resolved Hide resolved
docs/src/snap/explanation/datastore/k8s-dqlite.md Outdated Show resolved Hide resolved
}
```

2. Apply the new configuration using the k8sd API directly. You must run this
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe we should have a hidden command for that in the future.

@neoaggelos neoaggelos merged commit 4036489 into main Jul 3, 2024
17 checks passed
@neoaggelos neoaggelos deleted the KU-961/embedded branch July 3, 2024 13:47
@ktsakalozos ktsakalozos restored the KU-961/embedded branch July 3, 2024 15:56
ktsakalozos added a commit that referenced this pull request Jul 3, 2024
neoaggelos pushed a commit that referenced this pull request Jul 5, 2024
@eaudetcobello eaudetcobello deleted the KU-961/embedded branch August 28, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants