Added support for claiming nodes as part of installation. #10084
Conversation
github-actions
bot
added
the
area/packaging
Ferroin
force-pushed
the
installer-claiming
branch
from
7359ffc
to
c7617dc
Compare
Ferroin
force-pushed
the
installer-claiming
branch
2 times, most recently
from
1295d4b
to
9112ce3
Compare
Ferroin
force-pushed
the
installer-claiming
branch
from
ba3a13a
to
162efc1
Compare
There was a problem hiding this comment.
@Ferroin I tested the happy path both for installation through kickstart and through building the agent. It worked fine. I did not test corner cases plus have not tested it without setting the War Room parameter.
I have not reviewed the code.
Ferroin
dismissed stale reviews from knatsakis and manos-saratsis
via
1f13f0c
Ferroin
force-pushed
the
installer-claiming
branch
from
e07a420
to
1f13f0c
Compare
|
Rebased to fix the conflict in |
Ferroin
force-pushed
the
installer-claiming
branch
from
1f13f0c
to
606bade
Compare
As I understood at the time, the issue was how you were using the local-files option, not the code here, but you were’t getting an error message because of how the option parsing is handled (we can’t easily differentiate between parameters for an option and subsequent options). The full We really need to just split this out as a set of options instead of one option with five parameters, and probably make everything optional (and add support for all the other stuff we would need local copies of...). |
|
@Ferroin I see so this is a problem but not directly related to this PR.... Do you have a test plan here then? If I remember correctly kickstart here downloaded claim script from the master but this PR modifies the claim script... |
If you provide the full set of local files with that option, then it will use the claiming script in the provided source tarball. So the general test plan is to generate a source tarball from this PR using |
|
thx, I will retest that today then. Also, we should make a note to fix that option parsing (in future PR)... |
I’ve opened an issue to just redo the whole handling of local files. It’s a major pain point for some users, and the current implementation is horribly broken in a number of other ways. |
| @@ -25,6 +25,7 @@ if [ -n "${NETDATA_CLAIM_URL}" ] && [ -n "${NETDATA_CLAIM_TOKEN}" ] && [ ! -f /v | |||
| -url "${NETDATA_CLAIM_URL}" \ | |||
| ${NETDATA_CLAIM_ROOMS:+-rooms "${NETDATA_CLAIM_ROOMS}"} \ | |||
| ${NETDATA_CLAIM_PROXY:+-proxy "${NETDATA_CLAIM_PROXY}"} | |||
| -daemon-not-running | |||
There was a problem hiding this comment.
- That condition is not correct
i dont know what is /var/lib/netdata/claim.d/claimed_id. Perhaps some old dir?
- There is no
NETDATA_CLAIM_ROOMScheck. Is it ok to have no rooms. Does it work?
There was a problem hiding this comment.
- No, just me using the wrong name.
- AIUI from discussions about how claiming works that I0ve had with a couple of people, we do not need to specify rooms. Not doing so claims the node, but does not add it to any of the rooms in the space, but it can be manually added later through Netdata Cloud.
This adds four new options to the `netdata-installer.sh` script: * `--claim-token` * `--claim-rooms` * `--claim-uri` * `--claim-proxy` These directly correspond to the `-token`, `-rooms`, `-uri`, and `-proxy` options for the `netdata-claim.sh` script. They have the following associated logic: * If any are specified and the `--disable-cloud` option is also specified, we bail and tell the user to either enable the cloud or remove the claiming options. * If only some but not all of the token, rooms, and uri options are specified, we bail and tell the user that they must pass all three. * If all three of the token, rooms, and uri are specified, we invoke the `netdata-claim.sh` script for the install itself as one of the last steps in the installation process, using the values passed to these options. This allows users to directly claim the agent as part of the install, which is useful for automated installation scenarios.
This makes us more future-proof. The required changes also fix some buggy behavior in the option parsing code in the kickstart scripts.
These are leftovers from an earlier revision, they are not actually needed.
This lets it reliably claim nodes which have not yet had the daemon run. Also fixes a consistency issue in the claiming logic in the Docker entrypoint.
Ferroin
force-pushed
the
installer-claiming
branch
from
b8ff8fb
to
932b4cc
Compare
) * Added support for claiming nodes as part of installation. This adds four new options to the `netdata-installer.sh` script: * `--claim-token` * `--claim-rooms` * `--claim-uri` * `--claim-proxy` These directly correspond to the `-token`, `-rooms`, `-uri`, and `-proxy` options for the `netdata-claim.sh` script. They have the following associated logic: * If any are specified and the `--disable-cloud` option is also specified, we bail and tell the user to either enable the cloud or remove the claiming options. * If only some but not all of the token, rooms, and uri options are specified, we bail and tell the user that they must pass all three. * If all three of the token, rooms, and uri are specified, we invoke the `netdata-claim.sh` script for the install itself as one of the last steps in the installation process, using the values passed to these options. This allows users to directly claim the agent as part of the install, which is useful for automated installation scenarios. * Add missing space as suggested by @knatsakis * Properly handle installs in /. * Properly handle unprefixed installs. * Fix another spelling error in an option name. * Properly fix option naming. * Move claiming into kickstart script instead of netdata-installer. This makes us more future-proof. The required changes also fix some buggy behavior in the option parsing code in the kickstart scripts. * Fix checksums. * Sanely handle the daemon not running during the claiming process. * Silence incorrect shellcheck warning. * Simplify condition as suggested by @vkalintiris. * Clean up old changes that should not be here anymore. These are leftovers from an earlier revision, they are not actually needed. * Add ID generation logic to the claiming script. This lets it reliably claim nodes which have not yet had the daemon run. Also fixes a consistency issue in the claiming logic in the Docker entrypoint.
) * Added support for claiming nodes as part of installation. This adds four new options to the `netdata-installer.sh` script: * `--claim-token` * `--claim-rooms` * `--claim-uri` * `--claim-proxy` These directly correspond to the `-token`, `-rooms`, `-uri`, and `-proxy` options for the `netdata-claim.sh` script. They have the following associated logic: * If any are specified and the `--disable-cloud` option is also specified, we bail and tell the user to either enable the cloud or remove the claiming options. * If only some but not all of the token, rooms, and uri options are specified, we bail and tell the user that they must pass all three. * If all three of the token, rooms, and uri are specified, we invoke the `netdata-claim.sh` script for the install itself as one of the last steps in the installation process, using the values passed to these options. This allows users to directly claim the agent as part of the install, which is useful for automated installation scenarios. * Add missing space as suggested by @knatsakis * Properly handle installs in /. * Properly handle unprefixed installs. * Fix another spelling error in an option name. * Properly fix option naming. * Move claiming into kickstart script instead of netdata-installer. This makes us more future-proof. The required changes also fix some buggy behavior in the option parsing code in the kickstart scripts. * Fix checksums. * Sanely handle the daemon not running during the claiming process. * Silence incorrect shellcheck warning. * Simplify condition as suggested by @vkalintiris. * Clean up old changes that should not be here anymore. These are leftovers from an earlier revision, they are not actually needed. * Add ID generation logic to the claiming script. This lets it reliably claim nodes which have not yet had the daemon run. Also fixes a consistency issue in the claiming logic in the Docker entrypoint.
Summary
This adds four new options to the
kickstart.shandkickstart-static64.shscripts:--claim-token--claim-rooms--claim-url--claim-proxyThese directly correspond to the
-token,-rooms,-url, and-proxyoptions for thenetdata-claim.shscript. They have the following associated logic:--disable-cloudoption is also specified, we bail and tell the user to either enable the cloud orremove the claiming options.
netdata-claim.shscript after the installation is otherwise complete.This allows users to directly claim the agent as part of the install, which is useful for automated installation scenarios.
Component Name
area/packaging
Test Plan
This can be verified by using the above-mentioned options with a fresh install to claim nodes as they are installed. Only the modified kickstart scripts are needed for this, they will work with the existing stable and nightly builds.
Additional Information
Fixes: #8819
This also fixes some buggy behavior in the kickstart scripts’ option parsing code that would cause them to behave differently depending on option order.