Get the grains that we check on every startup from the start event #1833

admd · 2020-01-27T10:03:44Z

What does this PR change?

Get the grains (machine_id, saltboot_initrd, management_key) that we check on every startup from the start event instead of making a separate call.

Background

We faced performance issues during minions startup as machine_id grains is needed to lookup if minion already exists in the database. This is needed to differentiate between a startup and 1st-time registration.

We followed different approaches, each with some pros/cons. Those approaches are described herein this PR description https://github.com/SUSE/spacewalk/pull/8892. The related research card is https://github.com/SUSE/spacewalk/issues/8228 where one can see further discussion.

In the end, we introduced a patch in upstream which makes it possible to pass interesting grains in the payload of minion start-up event. Related PR saltstack/salt#54948.
We think this is better approach in a sense that, we don't need to make a separate call, of course, it has a downside that all the minions need to have this new parameter in their config so next time, when minion get started, we use grains from the start_up event instead of making a separate call.

It is also noticeable that I kept the old way as a fallback, the reason was the older minions might not get the new changes so we have to have a backup plan.

We save 1 salt call in case of already registered minions, even if startup grains are not configured.

So if start-up grains are supported, with this PR number of salt sync calls are following

Test	Before	After
New registration	2	1
Registered minion startup	2	0

If start-up grains are not supported, with this PR number of salt sync calls are following

Test	Before	After
New registration	2	2
Registered minion startup	2	1

Benchmarks

According to tests with 100 evil-minions at normal speed mode (--slowdown-factor 1) an ~1.32x speedup on the total time to handle all minion start events (29s on average versus 38.5s).

Note that it is expected that the slower the simulated minions, the bigger the speedup.

GUI diff

No difference.

DONE

Documentation

-Instead of a separate call, the grain is part of startup event, there is nothing to document here for the customer. I will add the documentation for using the helper `mgr_start_event_grains.sls state though
uyuni-project/uyuni-docs#88

DONE

Test coverage

No tests: Existing tests are modified
DONE

Links

Fixes # https://github.com/SUSE/spacewalk/issues/9909
Tracks # add downstream PR, if any

DONE

java/code/src/com/suse/manager/reactor/messaging/MinionStartupGrains.java

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

java/code/src/com/suse/manager/webui/utils/salt/MinionStartEvent.java

susemanager-utils/susemanager-sls/salt/util/mg_start_event_grains.sls

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

moio · 2020-01-29T09:41:49Z

@admd please remove the [WIP] tag from the title and re-request my review when ready

moio

Additional points:

WRT documentation: shouldn't we add an indication how to use susemanager-utils/susemanager-sls/salt/util/mgr_start_event_grains.sls? I guess it could be similar to what we added for the Salt mine or the fqdns grain
how do we make sure this new configuration is added by default on all new minions? We would need:

a change in susemanager-utils/susemanager-sls/salt/bootstrap/susemanager.conf for UI-bootstrapped minions, similar to what we did for the fqdns grain
a change in rhn_bootstrap_strings.py for script-bootstrapped minions similar to what we did for the fqdns grains, after finding out from a user we actually forgot
a change in sumaform's minion/init.sls to make life easier for testers

All of the above can optionally go into separate PRs linked from here. But I would not consider this whole "work item" done without them.

tests: that existing tests are modified is definitely needed, but I would also suggest adding at least minimal coverage to new code (eg. RegisterMinionEventMessage with non-empty grains parameter). I would say this needs addressing in this PR.
performance: I would really like to see some before/after numbers, maybe generated via evil-minions. How much do we gain in handling the startup event of, say, 1000 minions? Please contact me for any evil-minions questions, I am not sure the tool can be used as it is now to perform the test for this PR.

Meta: it was very difficult to review RegisterMinionEventMessageAction.java - not sure if it would have been possible, but splitting the change in multiple commits could have helped reviewers here, please keep in mind for next time. While you are fixing the code, double check that core logic in this file has not really changed with the refactorings that were necessary here.

java/spacewalk-java.changes

susemanager-utils/susemanager-sls/susemanager-sls.changes

java/code/src/com/suse/manager/webui/utils/salt/Grains.java

java/code/src/com/suse/manager/webui/utils/salt/MinionStartEvent.java

java/code/src/com/suse/manager/webui/services/impl/SaltService.java

java/code/src/com/suse/manager/reactor/test/RegisterMinionActionTest.java

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

admd · 2020-02-04T16:52:25Z

Thank you @moio for the review. I really overlooked some part. I have made the suggested changed. Please have a look. Now the only remaining part is the PR for sumaform & for doc. I will create them in a while.

aaannz · 2020-02-04T23:13:15Z

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

+        String oldMinionId = registeredMinion.getMinionId();
+        if (!minionId.equals(oldMinionId)) {
+            LOG.warn("Minion '" + oldMinionId + "' already registered, updating " +
+                    "profile to '" + minionId + "' [" + registeredMinion.getMachineId() + "]");
+            registeredMinion.setName(minionId);
+            registeredMinion.setMinionId(minionId);
+            ServerFactory.save(registeredMinion);
+            SystemManager.addHistoryEvent(registeredMinion, "Duplicate Machine ID", "Minion '" +
+                    oldMinionId + "' has been updated to '" + minionId + "'");
+
+            SALT_SERVICE.deleteKey(oldMinionId);


As you are touching this code, can you take a look at #1835, please.

moio

OK, this reads muuuch better now, thanks!

I still left a few remarks, of which only one is really important at this point.

Thanks for your perseverance!

spacewalk/certs-tools/rhn_bootstrap_strings.py

susemanager-utils/susemanager-sls/susemanager-sls.changes

java/code/src/com/suse/manager/reactor/test/RegisterMinionActionTest.java

java/code/src/com/suse/manager/webui/utils/salt/MinionStartupGrains.java

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

admd · 2020-02-05T16:36:06Z

Thank you @moio for the review. I have added 2 more commits with the changes. You can review them separately.

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

moio

LGTM, thanks for all the efforts here.

Please do run benchmarks on this and make sure you add a Speedup line in the PR's description.

moio · 2020-02-17T16:12:46Z

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

+                Optional<ActivationKey> activationKey =
+                        mkey.flatMap(mk -> ofNullable(ActivationKeyFactory.lookupByKey(mk)));
+                Optional<String> validReactivationKey =
+                        mkey.flatMap(mk -> isValidReactivationKey(activationKey, minionId) ? mkey : empty());


Style nitpick: Optional supports filter

Changed it. 👍

mcalmer

failing tests. Better to rebase and fix the tests first

admd · 2020-02-21T10:41:44Z

@mcalmer I have rebased the branch & you can review it now. There is some untreated rubocop error but rest should be fine.

mcalmer

Let's give it a try.

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

mcalmer · 2020-02-21T15:10:35Z

@admd a rebase should also solve most of the failing tests now.

java/code/src/com/suse/manager/webui/utils/salt/MinionStartupGrains.java

…on_start_grains

…y bootstrap script

admd requested review from moio and chiaradiamarcelo January 27, 2020 10:08

admd force-pushed the use-grains-passed-at-startup branch 4 times, most recently from 4a8bbec to 3eeecaa Compare January 27, 2020 12:46

aaannz reviewed Jan 27, 2020

View reviewed changes

admd changed the title ~~Get the machine-id grain from the start up event~~ [WIP]Get the machine-id grain from the start up event Jan 27, 2020

admd force-pushed the use-grains-passed-at-startup branch from 3eeecaa to e400c30 Compare January 28, 2020 13:42

admd force-pushed the use-grains-passed-at-startup branch from e400c30 to 69307d1 Compare January 30, 2020 10:32

admd changed the title ~~[WIP]Get the machine-id grain from the start up event~~ Get the machine-id grain from the start up event Jan 30, 2020

admd requested review from aaannz and moio and removed request for moio January 30, 2020 10:40

moio suggested changes Feb 3, 2020

View reviewed changes

admd changed the title ~~Get the machine-id grain from the start up event~~ Get the grains that we check on every startup from the start event Feb 4, 2020

admd requested a review from moio February 4, 2020 16:53

aaannz reviewed Feb 4, 2020

View reviewed changes

moio reviewed Feb 5, 2020

View reviewed changes

admd requested a review from hustodemon February 5, 2020 12:18

This was referenced Feb 5, 2020

Add config param to pass grains to the minion start event uyuni-project/sumaform#678

Merged

Added the guidlines to enable 'start_event_grains' config uyuni-project/uyuni-docs#88

Merged

moio reviewed Feb 6, 2020

View reviewed changes

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java Outdated Show resolved Hide resolved

moio reviewed Feb 7, 2020

View reviewed changes

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java Outdated Show resolved Hide resolved

admd force-pushed the use-grains-passed-at-startup branch from 3cc38b6 to d5917f6 Compare February 7, 2020 17:45

github-actions bot added java java_lint_checkstyle labels Feb 12, 2020

github-actions bot added the java_pgsql_tests label Feb 12, 2020

moio approved these changes Feb 17, 2020

View reviewed changes

admd force-pushed the use-grains-passed-at-startup branch from d5917f6 to ae33f6b Compare February 18, 2020 09:53

admd requested a review from mcalmer February 18, 2020 09:56

mcalmer requested changes Feb 19, 2020

View reviewed changes

admd force-pushed the use-grains-passed-at-startup branch from ae33f6b to 483e566 Compare February 21, 2020 10:31

mcalmer approved these changes Feb 21, 2020

View reviewed changes

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java Outdated Show resolved Hide resolved

aaannz reviewed Feb 21, 2020

View reviewed changes

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java Outdated Show resolved Hide resolved

admd force-pushed the use-grains-passed-at-startup branch from 483e566 to acb0b9a Compare February 21, 2020 14:11

aaannz approved these changes Feb 21, 2020

View reviewed changes

admd force-pushed the use-grains-passed-at-startup branch from acb0b9a to 8cddc59 Compare February 21, 2020 15:14

aaannz reviewed Feb 24, 2020

View reviewed changes

java/code/src/com/suse/manager/webui/utils/salt/MinionStartupGrains.java Show resolved Hide resolved

admd added 8 commits February 25, 2020 08:57

get the machine-id grain from the start up event

84e419e

add a helper sls file to update the minion config to make use of mini…

35a8f14

…on_start_grains

Provided builder to construct object easily for testing

40b88f2

added new tests to cover scenerios when startup grains are passed

e31ed92

Modify existings tests to make use of new builder

df0ea59

added susemanager and saltboot_initrd grains to the state file

2f58e22

added config so new bootsrapped minions can get the config automatically

6fbc34b

Add 'start_event_grains' minion option to configfile when generated b…

a0d897b

…y bootstrap script

moio force-pushed the use-grains-passed-at-startup branch 2 times, most recently from 69307d1 to a0d897b Compare February 25, 2020 08:00

moio merged commit ee453d9 into uyuni-project:master Feb 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get the grains that we check on every startup from the start event #1833

Get the grains that we check on every startup from the start event #1833

admd commented Jan 27, 2020 •

edited by moio

Loading

moio commented Jan 29, 2020

moio left a comment

admd commented Feb 4, 2020

aaannz Feb 4, 2020

moio left a comment

admd commented Feb 5, 2020

moio left a comment

moio Feb 17, 2020

admd Feb 18, 2020

mcalmer left a comment

admd commented Feb 21, 2020

mcalmer left a comment

mcalmer commented Feb 21, 2020

Get the grains that we check on every startup from the start event #1833

Get the grains that we check on every startup from the start event #1833

Conversation

admd commented Jan 27, 2020 • edited by moio Loading

What does this PR change?

Background

Benchmarks

GUI diff

Documentation

Test coverage

Links

moio commented Jan 29, 2020

moio left a comment

Choose a reason for hiding this comment

admd commented Feb 4, 2020

aaannz Feb 4, 2020

Choose a reason for hiding this comment

moio left a comment

Choose a reason for hiding this comment

admd commented Feb 5, 2020

moio left a comment

Choose a reason for hiding this comment

moio Feb 17, 2020

Choose a reason for hiding this comment

admd Feb 18, 2020

Choose a reason for hiding this comment

mcalmer left a comment

Choose a reason for hiding this comment

admd commented Feb 21, 2020

mcalmer left a comment

Choose a reason for hiding this comment

mcalmer commented Feb 21, 2020

admd commented Jan 27, 2020 •

edited by moio

Loading