Support Patternless kubernetes and crio packages #1178

evrardjp · 2020-06-19T17:05:39Z

Without this patch, skuba is not able to install the new
kind of packages (patternless).

This is a problem, as it's necessary to install caasp5.

This also tackles the upgrade case, where we remove
the previous packages, and install the new kind of pkgs.

Partial-Fixes: https://github.com/SUSE/avant-garde/issues/1663

evrardjp

Notes

go.mod

internal/pkg/skuba/deployments/ssh/kubernetes.go

evrardjp · 2020-06-19T17:08:02Z

internal/pkg/skuba/deployments/ssh/zypper.go

+	// Runs a zypper install command wrapped with the right userdata and parameters
+	var cliArgs []string
+	cliArgs = append(cliArgs, "--userdata", "skuba")
+	cliArgs = append(cliArgs, "--non-interactive", "install", "--")


note self, that looks silly, maybe put it in one line.

evrardjp · 2020-06-22T15:00:19Z

Rebased to include latest master changes, including the necessary 1.13 support.

evrardjp · 2020-06-23T08:00:12Z

Retriggered CI now that the build service is configured with the correct repos.

evrardjp · 2020-06-23T15:14:36Z

recheck

evrardjp · 2020-06-23T16:14:57Z

After a discussion at our standup today, we discussed about having extra feedback on patternless.

I discussed with Dirk today, and had a long chat/etherpad about alternatives (See it here https://etherpad.opendev.org/p/O9Ds7qdMcofMJdfsYyns )

I am now testing a PoC which doesn't imply Conflicts, only Requires, and Obsoletes, with different names. If that PoC is fine, I will update the packages and this PR, as it might mean some of the removals in here might not be needed anymore.

evrardjp · 2020-06-25T15:03:12Z

Sadly, we went down the rabbithole with a weird behaviour in libsolv for the Obsoletes.
We're gonna drop that idea, and keep this patch idea the same.
The Patternless and versionappended therefore seems still the way to go.

However, as explained in the etherpad, we found a bug in the cri-o handling. This means that CRI-O also need to be versionappended.

This also means that "patternless" is harder, and might not make sense anymore!

However, the spirit behind patternless to simplify and be "versionappended" seems now more than ever necessary. I just fail to name/brand it properly :)
I am discussing with dirk if we should bring patterns back with versionappended cri-o and kubernetes, or if we keep patternless versionappended crio and k8s.

In the meantime, I have updated the PR to do patternless versionappended.

evrardjp · 2020-06-26T12:35:16Z

internal/pkg/skuba/deployments/ssh/zypper.go

-		return false, err
-	}
-	return true, nil
+func (t *Target) zypperInstall(packages ...string) (stdout string, stderr string, error error) {


note to self, might want to expose this function.

evrardjp · 2020-06-26T15:07:31Z

skuba-update requires any version of kubernetes client, therefore the packaging of kubernetes-<version>-client should also provide kubernetes-client. I have updated the package. I will retrigger the build when the package is published.

evrardjp · 2020-06-26T16:39:28Z

Seems like the greenfield empty kubernetes deployed just fine, but there was a problem with the addons. I will check what's going on on Monday

flavio

LGTM so far, I haven't tested. I've left some comments along the way

internal/pkg/skuba/deployments/ssh/kubernetes.go

flavio · 2020-06-29T16:10:47Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+	pkgs = append(pkgs, fmt.Sprintf("+kubernetes-%s-kubeadm", current))
+	pkgs = append(pkgs, fmt.Sprintf("+kubernetes-%s-kubelet", current))
+	pkgs = append(pkgs, fmt.Sprintf("+kubernetes-%s-client", current))
+	pkgs = append(pkgs, fmt.Sprintf("+cri-o-%s*", current))


Maybe we could put these patterns inside of a constant list to reuse them later inside of the code (eg: kubernetesUpgradeOtherPkgs)?

Maybe two lists: phase1pkgs and phase2kpgs 🤔

I hesitated there, because I have the impression they will be fluctuating over time. But indeed, they aren't right now, so that might make sense.

Let's keep that for now, if you don't mind.

internal/pkg/skuba/deployments/ssh/kubernetes.go

Without this patch, skuba is not able to install the new kind of packages (patternless). This is a problem, as it is necessary to install caasp5 this way, which itself is necessary because migrations from caasp4 to caasp5 break clusters by auto-upgrading to kubernetes 1.18 and cri-o without using skuba. This patch tackles both the greenfield and the upgrade cases, where we remove the previous packages, and install the new kind of pkgs. Partial Fix: SUSE/avant-garde#1663

evrardjp · 2020-06-29T16:34:37Z

I have resolved @flavio 's comments/concerns, and rebased this to test how this fares with Cilium 1.7 changes.

evrardjp · 2020-06-30T06:50:15Z

I think we should go ahead and merge this PR and the packages into Devel:CaaSP:5, remove the previous kubernetes/cri-o packages in that repo.

kkaempf · 2020-06-30T07:10:39Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

@@ -56,24 +58,102 @@ func kubernetesUploadSecrets(errorHandling KubernetesUploadSecretsErrorBehavior)
 	}
 }

-func kubernetesInstallNodePattern(t *Target, data interface{}) error {


I'm missing a comment here, explaining the new return parameters

kkaempf · 2020-06-30T07:13:39Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+
+	var pkgs []string
+
+	if currentV == "1.17" {


This is brittle and shouldn't be buried in the code. There should be a LAST_CAASP_4_K8S_VERSION (or so) constant.

I can have that 1.17 as a constant, no problem. I suggest to do that as a follow up PR, to not delay this further.

kkaempf · 2020-06-30T07:15:20Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+		// On 1.17 we can't remove kubernetes-1.17-kubeadm, because it doesn't exist.
+		// Removing kubeadm keeps kubelet alive.
+		// The rest needs to be removed on the next stage.
+		pkgs = append(pkgs, "-patterns-caasp-Node-1.17", "-kubernetes-kubeadm", "-cri-o-kubeadm-criconfig")


Frankly, this makes me cringe. Removal of v4 patterns/packages should be handled by Obsoletes imho.

While this is a noble thing we tried, and should be working, we arrived to a libsolv issue/feature that was created for bad packagers, where an obsolete-ing package is considered as an upgrade candidate for the obsolete-d package (the tl;dr:). We can discuss this for later, but right now it works, and is user friendly enough IMO

kkaempf · 2020-06-30T07:15:44Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+
+	var pkgs []string
+
+	if currentV == "1.17" {


see comment above

Agreed. Will do so in a follow up PR.

evrardjp

I think we should go ahead, and fine tune this in a follow up PR. No need to retrigger all the tests and delay this patch further for cosmetics.

evrardjp · 2020-06-30T07:21:30Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+		// On 1.17 we can't remove kubernetes-1.17-kubeadm, because it doesn't exist.
+		// Removing kubeadm keeps kubelet alive.
+		// The rest needs to be removed on the next stage.
+		pkgs = append(pkgs, "-patterns-caasp-Node-1.17", "-kubernetes-kubeadm", "-cri-o-kubeadm-criconfig")


While this is a noble thing we tried, and should be working, we arrived to a libsolv issue/feature that was created for bad packagers, where an obsolete-ing package is considered as an upgrade candidate for the obsolete-d package (the tl;dr:). We can discuss this for later, but right now it works, and is user friendly enough IMO

evrardjp · 2020-06-30T07:23:14Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+
+	var pkgs []string
+
+	if currentV == "1.17" {


I can have that 1.17 as a constant, no problem. I suggest to do that as a follow up PR, to not delay this further.

evrardjp · 2020-06-30T07:23:35Z

internal/pkg/skuba/deployments/ssh/kubernetes.go

+
+	var pkgs []string
+
+	if currentV == "1.17" {


Agreed. Will do so in a follow up PR.

kkaempf · 2020-06-30T07:30:36Z

Please create a github issue as a reminder for the follow up PR and reference this issue here. Then I'm fine to merge.

evrardjp · 2020-06-30T07:31:41Z

I will just create another PR on top right after my meetings this morning.

mssola

Besides some comments that have already been noted, this looks good to me. But make sure to polish this later on.

chentex

Seems that everyone is now on the same page

evrardjp commented Jun 19, 2020

View reviewed changes

evrardjp added the ci-repo:pattern-less label Jun 22, 2020

evrardjp force-pushed the patternless branch 3 times, most recently from 3657add to 9df27a8 Compare June 22, 2020 14:56

evrardjp force-pushed the patternless branch 2 times, most recently from cee71d8 to 5bfdaad Compare June 23, 2020 07:59

evrardjp force-pushed the patternless branch from 5bfdaad to 1d7dd99 Compare June 24, 2020 10:37

evrardjp changed the title ~~[WIP] Support Patternless kubernetes packages~~ [WIP] Support Patternless kubernetes and crio packages Jun 25, 2020

evrardjp force-pushed the patternless branch from 1d7dd99 to cccb49a Compare June 25, 2020 14:51

evrardjp force-pushed the patternless branch 3 times, most recently from 5a07fe3 to 1d57a58 Compare June 26, 2020 12:03

evrardjp requested a review from flavio June 26, 2020 12:36

evrardjp commented Jun 26, 2020

View reviewed changes

evrardjp force-pushed the patternless branch 2 times, most recently from 01f98e6 to 53a82dd Compare June 26, 2020 14:30

evrardjp force-pushed the patternless branch from 53a82dd to 27656a8 Compare June 26, 2020 15:31

evrardjp force-pushed the patternless branch from 263c451 to 7d2e3ea Compare June 29, 2020 14:57

evrardjp changed the title ~~[WIP] Support Patternless kubernetes and crio packages~~ Support Patternless kubernetes and crio packages Jun 29, 2020

flavio reviewed Jun 29, 2020

View reviewed changes

evrardjp force-pushed the patternless branch from 7d2e3ea to 2966e8f Compare June 29, 2020 16:31

kkaempf reviewed Jun 30, 2020

View reviewed changes

evrardjp commented Jun 30, 2020

View reviewed changes

mssola approved these changes Jun 30, 2020

View reviewed changes

chentex approved these changes Jun 30, 2020

View reviewed changes

evrardjp mentioned this pull request Jun 30, 2020

Patternless cosmetic cleanups #1210

Merged

davidcassany mentioned this pull request Jun 30, 2020

Make use of the new versioned k8s packages SUSE/caasp-container-images#90

Merged

evrardjp merged commit 871aaad into SUSE:master Jun 30, 2020

evrardjp deleted the patternless branch June 30, 2020 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Patternless kubernetes and crio packages #1178

Support Patternless kubernetes and crio packages #1178

evrardjp commented Jun 19, 2020 •

edited

Loading

evrardjp left a comment

evrardjp Jun 19, 2020

evrardjp commented Jun 22, 2020

evrardjp commented Jun 23, 2020

evrardjp commented Jun 23, 2020

evrardjp commented Jun 23, 2020 •

edited

Loading

evrardjp commented Jun 25, 2020

evrardjp Jun 26, 2020

evrardjp commented Jun 26, 2020 •

edited

Loading

evrardjp commented Jun 26, 2020

flavio left a comment

flavio Jun 29, 2020

evrardjp Jun 29, 2020

evrardjp Jun 29, 2020

evrardjp commented Jun 29, 2020

evrardjp commented Jun 30, 2020

kkaempf Jun 30, 2020

kkaempf Jun 30, 2020

evrardjp Jun 30, 2020

kkaempf Jun 30, 2020

evrardjp Jun 30, 2020

kkaempf Jun 30, 2020

evrardjp Jun 30, 2020

evrardjp left a comment

evrardjp Jun 30, 2020

evrardjp Jun 30, 2020

evrardjp Jun 30, 2020

kkaempf commented Jun 30, 2020

evrardjp commented Jun 30, 2020

mssola left a comment

chentex left a comment

Support Patternless kubernetes and crio packages #1178

Support Patternless kubernetes and crio packages #1178

Conversation

evrardjp commented Jun 19, 2020 • edited Loading

evrardjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrardjp commented Jun 22, 2020

evrardjp commented Jun 23, 2020

evrardjp commented Jun 23, 2020

evrardjp commented Jun 23, 2020 • edited Loading

evrardjp commented Jun 25, 2020

Choose a reason for hiding this comment

evrardjp commented Jun 26, 2020 • edited Loading

evrardjp commented Jun 26, 2020

flavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrardjp commented Jun 29, 2020

evrardjp commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrardjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkaempf commented Jun 30, 2020

evrardjp commented Jun 30, 2020

mssola left a comment

Choose a reason for hiding this comment

chentex left a comment

Choose a reason for hiding this comment

evrardjp commented Jun 19, 2020 •

edited

Loading

evrardjp commented Jun 23, 2020 •

edited

Loading

evrardjp commented Jun 26, 2020 •

edited

Loading