Switch microk8s enable wrapper to Python script #1467

knkski · 2020-08-03T19:42:58Z

For now, it's just a clone of the old bash script, with the ability to handle unix-style flag arguments. Using click however, will allow expanding the enable scripts to be full click subcommands, instead of just bash scripts.

Fixes #1446

microk8s-resources/wrappers/microk8s-enable.wrapper

johnsca

Seems reasonable to me, aside from handling the one edge case that I mentioned a bit more gracefully (unless I'm missing something about how click works).

balchua · 2020-08-03T21:39:37Z

@knkski do we drop the checking for addons if they are already running? If yes, i think this is required, sometimes users manually edit the resource, and when the user enabled it again, it loses their previous changes.

knkski · 2020-08-04T15:32:28Z

@balchua: The use case for dropping that check is a result of #1397. If a user enables kubeflow and it either fails or the kubeflow namespace is later deleted, a user will be unable to either enable or disable kubeflow.

It would be nice to have microk8s.enable kubeflow support redeploying in case of a failed enable attempt, but maybe there's a better way to handle that. @ktsakalozos, @balchua: thoughts? cc @RFMVasconcelos, who I think also had opinions on that.

balchua · 2020-08-04T21:28:24Z

Thanks @knkski for the info. Instead of dropping that altogether, maybe we can consider adding idempotent: true or false into the addon-list.yaml? If idempotent is true then we dont reapply otherwise we reapply.
Another option i can think of is an option --force that users can pass to the enable action. Example:
microk8s enable --force kubeflow
These are just options on top of my head.

ktsakalozos · 2020-08-10T10:55:02Z

sometimes users manually edit the resource, and when the user enabled it again, it loses their previous changes.

The enable script used to reapply the manifests without checking if the addon was already enabled. This was causing some confusion [2] and we had to fix it [1].

@knkski kubeflow enables the dns addon but I also think it expects the forward dns to be correctly set. Will the kubeflow deploy in an environment where 8.8.8.8 is not reachable?

[1] #927
[2] #958

balchua · 2020-08-10T11:56:03Z

So does it mean that enable command do not need to check whether it is enabled or not?

ktsakalozos · 2020-08-10T12:04:58Z

Sorry I was not clear. We need to keep the current behavior of not re-applying the addon manifests. We have to respect any configuration users have on the running workloads.

balchua · 2020-08-10T12:10:36Z

Aaa ok. Thanks for clarifying.

knkski · 2020-08-19T18:19:54Z

I've updated this PR so that it keeps the current behavior of not enabling already-enabled addons, for everything but Kubeflow, which has it's own logic for handling that. Namely, it will allow disabling Kubeflow even if Kubeflow is already disabled, which lets us fix #1397.

I didn't create a idempotent: true section in addon-list.yaml, opting instead for a hardcoded check for the kubeflow addon, but I could add that if desired.

balchua · 2020-08-19T21:35:04Z

Thanks @knkski. Im good with that. The unix style args is nice.

microk8s-resources/wrappers/microk8s-disable.wrapper

ktsakalozos · 2020-11-18T09:34:40Z

@knkski there are a few conflicts in this PR. Do you think you could address them so we get this PR ready for 1.20? Thank you.

For now, they're just clones of the old bash scripts, with the ability to handle unix-style flag arguments. Using click however, will allow expanding the enable scripts to be full click subcommands, instead of just bash scripts.

ktsakalozos · 2020-11-26T08:08:09Z

scripts/wrappers/common/utils.py

+
+        if addon not in existing_addons:
+            click.secho("Addon `%s` not found." % addon, fg='red', err=True)
+            click.echo("The available addons are:\n - %s" % '\n - '.join(existing_addons), err=True)


I am not so sure about this. If we print the list of addons here I easily see a request to put descriptions next to the addon names.

ktsakalozos · 2020-11-26T08:09:20Z

scripts/wrappers/common/utils.py

-        exit(0)
+    if (snap_data() / 'var/lock/clustered.lock').exists():
+        click.echo('This MicroK8s deployment is acting as a node in a cluster.')
+        click.echo('Please use `microk8s enable` on the master.')


Based on the error message, I assume method was used by microk8s.status.

ktsakalozos · 2020-11-26T08:21:16Z

scripts/wrappers/common/utils.py

+        if args and addons[1:]:
+            click.secho(
+                click.style(
+                    "Can't pass string arguments and flag arguments simultaneously!\n", fg='red'


I like what you are doing here, error messages in red. Could we do this across all microk8s in a separate PR? If we do this coloring only here it will look odd.

This reverts commit a0ac699.

ktsakalozos · 2020-11-27T14:50:32Z

Just reverted this PR, there are a few things (probably minor) that will need a bit more attention.

On enable/disable we request the status of each addon [1]. We should first make sure the API server is running or else the get status fails with:

14:55:29 2020-11-27 at 12:55:29 | INFO + /snap/microk8s/1829/actions/enable.cilium.sh
14:55:29 2020-11-27 at 12:55:29 | INFO Traceback (most recent call last):
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/scripts/wrappers/enable.py", line 32, in <module>
14:55:29 2020-11-27 at 12:55:29 | INFO enable(prog_name='microk8s enable')
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/usr/lib/python3/dist-packages/click/core.py", line 716, in __call__
14:55:29 2020-11-27 at 12:55:29 | INFO return self.main(*args, **kwargs)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/usr/lib/python3/dist-packages/click/core.py", line 696, in main
14:55:29 2020-11-27 at 12:55:29 | INFO rv = self.invoke(ctx)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/usr/lib/python3/dist-packages/click/core.py", line 889, in invoke
14:55:29 2020-11-27 at 12:55:29 | INFO return ctx.invoke(self.callback, **ctx.params)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/usr/lib/python3/dist-packages/click/core.py", line 534, in invoke
14:55:29 2020-11-27 at 12:55:29 | INFO return callback(*args, **kwargs)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/scripts/wrappers/enable.py", line 25, in enable
14:55:29 2020-11-27 at 12:55:29 | INFO enabled_addons, _ = get_status(get_available_addons(get_current_arch()), True)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/scripts/wrappers/status.py", line 157, in get_status
14:55:29 2020-11-27 at 12:55:29 | INFO kube_output = kubectl_get("all")
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/scripts/wrappers/common/utils.py", line 169, in kubectl_get
14:55:29 2020-11-27 at 12:55:29 | INFO return run("kubectl", kubeconfig, "get", cmd, "--all-namespaces", die=False)
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/scripts/wrappers/common/utils.py", line 39, in run
14:55:29 2020-11-27 at 12:55:29 | INFO result.check_returncode()
14:55:29 2020-11-27 at 12:55:29 | INFO File "/snap/microk8s/1829/usr/lib/python3.5/subprocess.py", line 659, in check_returncode
14:55:29 2020-11-27 at 12:55:29 | INFO self.stderr)
14:55:29 2020-11-27 at 12:55:29 | INFO subprocess.CalledProcessError: Command '('kubectl', '--kubeconfig=/var/snap/microk8s/1829/credentials/client.config', 'get', 'all', '--all-namespaces')' returned non-zero exit status 1

Similar case is when we enable/disable addons. We used to wait for up to 30 seconds for the API server to get ready [2]. This way if an addon enables/disables the apiserver we would not fail in [3].

The last thing that needs some mending is the --help. Most of the addons do not have a --help so we are pushing the users to this wrong path:

ubuntu@ip-172-31-40-63:~$ microk8s.disable --help
Usage: microk8s disable [OPTIONS] ADDONS...

  Disables one or more MicroK8s addons.

  For a list of available addons, run `microk8s status`.

  To see help for individual addons, run:

      microk8s disable ADDON -- --help

Options:
  --help  Show this message and exit.
ubuntu@ip-172-31-40-63:~$ microk8s.disable dns -- --help
Disabling DNS
Reconfiguring kubelet
Removing DNS manifest
serviceaccount "coredns" deleted
configmap "coredns" deleted
deployment.apps "coredns" deleted
service "kube-dns" deleted
clusterrole.rbac.authorization.k8s.io "coredns" deleted
clusterrolebinding.rbac.authorization.k8s.io "coredns" deleted
DNS is disabled

We should get back the old help message.

[1] https://github.com/ubuntu/microk8s/pull/1467/files#diff-44f8a77098734624eb59a3c59e17024d622e15fb9927eb4d6f3583ac724ffe25R25
[2] https://github.com/ubuntu/microk8s/blob/master/microk8s-resources/wrappers/microk8s-enable.wrapper#L31
[3] https://github.com/ubuntu/microk8s/pull/1467/files#diff-cea8894ac12cb68765874a03fba91e79d22c596d86633886b5c02abe52829448R246

knkski requested review from joedborg, ktsakalozos and balchua August 3, 2020 19:42

johnsca reviewed Aug 3, 2020

View reviewed changes

microk8s-resources/wrappers/microk8s-enable.wrapper Outdated Show resolved Hide resolved

johnsca approved these changes Aug 3, 2020

View reviewed changes

knkski force-pushed the python-enable-script branch from 614f408 to b400c65 Compare August 3, 2020 21:12

knkski force-pushed the python-enable-script branch from b400c65 to 2042819 Compare August 3, 2020 21:49

knkski force-pushed the python-enable-script branch from 2042819 to 61e8790 Compare August 4, 2020 15:36

joedborg approved these changes Aug 11, 2020

View reviewed changes

knkski force-pushed the python-enable-script branch 14 times, most recently from 4a8a384 to 02fb591 Compare August 15, 2020 18:35

knkski force-pushed the python-enable-script branch 3 times, most recently from 8c43cf7 to 7a4cc8a Compare August 28, 2020 15:53

ktsakalozos reviewed Nov 18, 2020

View reviewed changes

microk8s-resources/wrappers/microk8s-disable.wrapper Show resolved Hide resolved

knkski force-pushed the python-enable-script branch 5 times, most recently from ffc330b to b0617e1 Compare November 20, 2020 22:50

knkski requested review from ktsakalozos, joedborg and johnsca November 23, 2020 19:17

knkski added 2 commits November 25, 2020 08:56

Switch microk8s enable/disable wrappers to Python script

fdb8158

For now, they're just clones of the old bash scripts, with the ability to handle unix-style flag arguments. Using click however, will allow expanding the enable scripts to be full click subcommands, instead of just bash scripts.

Add extra calico check in CI

9de59d1

knkski force-pushed the python-enable-script branch from ea7a79f to 9de59d1 Compare November 25, 2020 14:57

ktsakalozos reviewed Nov 26, 2020

View reviewed changes

ktsakalozos added 3 commits November 26, 2020 11:16

Style fixes

0b57ff4

Black style changes

39d4d7c

More black style changes

835b2a3

ktsakalozos merged commit a0ac699 into master Nov 26, 2020

ktsakalozos added a commit that referenced this pull request Nov 27, 2020

Revert "Switch microk8s enable wrapper to Python script (#1467)"

9b8c8e8

This reverts commit a0ac699.

ktsakalozos mentioned this pull request Nov 27, 2020

Revert "Switch microk8s enable wrapper to Python script" #1778

Merged

ktsakalozos added a commit that referenced this pull request Nov 27, 2020

Revert "Switch microk8s enable wrapper to Python script (#1467)" (#1778)

e63a30f

This reverts commit a0ac699.

knkski mentioned this pull request Dec 1, 2020

Switch microk8s enable/disable wrappers to Python script #1782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch microk8s enable wrapper to Python script #1467

Switch microk8s enable wrapper to Python script #1467

knkski commented Aug 3, 2020

johnsca left a comment

balchua commented Aug 3, 2020

knkski commented Aug 4, 2020

balchua commented Aug 4, 2020

ktsakalozos commented Aug 10, 2020

balchua commented Aug 10, 2020

ktsakalozos commented Aug 10, 2020

balchua commented Aug 10, 2020

knkski commented Aug 19, 2020

balchua commented Aug 19, 2020

ktsakalozos commented Nov 18, 2020

ktsakalozos Nov 26, 2020

ktsakalozos Nov 26, 2020

ktsakalozos Nov 26, 2020

ktsakalozos commented Nov 27, 2020

Switch microk8s enable wrapper to Python script #1467

Switch microk8s enable wrapper to Python script #1467

Conversation

knkski commented Aug 3, 2020

johnsca left a comment

Choose a reason for hiding this comment

balchua commented Aug 3, 2020

knkski commented Aug 4, 2020

balchua commented Aug 4, 2020

ktsakalozos commented Aug 10, 2020

balchua commented Aug 10, 2020

ktsakalozos commented Aug 10, 2020

balchua commented Aug 10, 2020

knkski commented Aug 19, 2020

balchua commented Aug 19, 2020

ktsakalozos commented Nov 18, 2020

ktsakalozos Nov 26, 2020

Choose a reason for hiding this comment

ktsakalozos Nov 26, 2020

Choose a reason for hiding this comment

ktsakalozos Nov 26, 2020

Choose a reason for hiding this comment

ktsakalozos commented Nov 27, 2020