Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Allow any version of k8s-client for skuba-update and allow cri-o bump during stage1 #1227

Merged
merged 3 commits into from
Jul 7, 2020

Conversation

evrardjp
Copy link
Contributor

@evrardjp evrardjp commented Jul 6, 2020

With the change of the virtual provides from kubernetes- to
k8s- and the version appended packages, we can now to decide
whether this package need any version of a kubernetes, or
a specific one. skuba-update can rely on any version of kubectl,
therefore we should be updating the spec to rely on k8s-client
being present.

Why is this PR needed?

Without this whole virtual provides, the migration will auto bump k8s to 1.18, which we want to avoid.

What does this PR do?

Ensure we are not using kubernetes-component* packages.

Anything else a reviewer needs to know?

Special test cases, manual steps, links to resources or anything else that could be helpful to the reviewer.

Info for QA

This is info for QA so that they can validate this. This is mandatory if this PR fixes a bug.
If this is a new feature, a good description in "What does this PR do" may be enough.

Related info

Info that can be relevant for QA:

  • link to other PRs that should be merged together
  • link to packages that should be released together
  • upstream issues

Status BEFORE applying the patch

How can we reproduce the issue? How can we see this issue? Please provide the steps and the prove
this issue is not fixed.

Status AFTER applying the patch

How can we validate this issue is fixed? Please provide the steps and the prove this issue is fixed.

Docs

If docs need to be updated, please add a link to a PR to https://github.com/SUSE/doc-caasp.
At the time of creating the issue, this PR can be work in progress (set its title to [WIP]),
but the documentation needs to be finalized before the PR can be merged.

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

mssola
mssola previously approved these changes Jul 6, 2020
@kkaempf
Copy link
Member

kkaempf commented Jul 6, 2020

lgtm !

@evrardjp
Copy link
Contributor Author

evrardjp commented Jul 6, 2020

Retriggering, as kubernetes was not properly built in the repo when this PR was up.

@@ -60,7 +60,7 @@ Summary: Utility to automatically refresh and update a skuba cluster
Group: System/Management
Requires: python3-setuptools
Requires: zypper >= 1.14.15
Requires: kubernetes-client
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent the upgrade of the client without reason, we should not do this.

Instead we should remove the requirement, and be explicit about possible breakages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the removal breaks the greenfields's skuba-update (which I doubt we have test coverage for), then we can probably iterate with Requires: (kubernetes-client or k8s-client)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Requires of (kubernetes-client or k8s-client) triggers the install of kubernetes-1.18-client during zypper dup, so I removed this require.

@evrardjp
Copy link
Contributor Author

evrardjp commented Jul 6, 2020

Please hold. Don't merge this yet.

With the change of the virtual provides from kubernetes- to
k8s- and the version appended packages, we can now to decide
whether this package need _any_ version of a kubernetes, or
a specific one. skuba-update can rely on any version of kubectl,
therefore we should be updating the spec to rely on k8s-client
possibly being present.
@evrardjp evrardjp dismissed stale reviews from jordimassaguerpla and mssola via fd59c96 July 6, 2020 15:04
@evrardjp evrardjp force-pushed the update-virtual-provides-to-k8s branch from a5ce53e to fd59c96 Compare July 6, 2020 15:04
@evrardjp evrardjp changed the title Allow any version of k8s-client Allow any version of k8s-client for skuba-update and allow cri-o bump during stage1 Jul 6, 2020
Without this patch, the removal of kubernetes-kubeadm will be
problematic, as no packages will provide it and it is still
required by cri-o-kubeadm-criconfig. If we remove this
package, and upgrade cri-o to 1.18 during the same transaction,
we'll keep the kubelet to the current version, and kubeadm can
be upgraded to 1.18.
@evrardjp evrardjp force-pushed the update-virtual-provides-to-k8s branch from fd59c96 to 0e0c8e4 Compare July 7, 2020 09:18
Copy link
Collaborator

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

LGTM

// package to avoid upgrade during zypper migration).
// we need to remove cri-o in stage2 else 1.17 kubelet could
// complain about cri-runtime being absent.
pkgs = append(pkgs, "-patterns-caasp-Node-1.17", "-\"kubernetes-kubeadm<1.18\"", "-caasp-config", "-cri-o-kubeadm-criconfig")
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I didn't know zypper was capable to understand a syntax such as -kubernetes-kubeadm<1.18 😉

Copy link
Contributor Author

@evrardjp evrardjp Jul 7, 2020

Choose a reason for hiding this comment

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

@davidcassany davidcassany merged commit fa410d6 into SUSE:master Jul 7, 2020
@evrardjp evrardjp deleted the update-virtual-provides-to-k8s branch July 7, 2020 15:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants