Skip to content
This repository was archived by the owner on May 30, 2023. It is now read-only.

app-admin/etcd-wrapper: remove ETCD_NAME#1444

Merged
tormath1 merged 1 commit intomainfrom
tormath1/etcd
Nov 29, 2021
Merged

app-admin/etcd-wrapper: remove ETCD_NAME#1444
tormath1 merged 1 commit intomainfrom
tormath1/etcd

Conversation

@tormath1
Copy link
Copy Markdown
Contributor

@tormath1 tormath1 commented Nov 24, 2021

etcd node's name was defined by ETCD_NAME, from etcd/v3 the server
can't be started with both ETCD_NAME and --name supplied.

Which leads to three cases:

  • etcd-member.service starts without further configuration, no issue
    since only ETCD_NAME=%m is used
  • etcd-member.service is overrided with a CLC without name: key, no
    issue since only ETCD_NAME=%m is used
  • etcd-member.service is overrided with a CLC with a name: key,
    there is an issue since in the final service we will have both
    ETCD_NAME=%m and --name name-from-clc

This patch conditionally unset the ETCD_NAME in case --name/-name is
supplied.

Signed-off-by: Mathieu Tortuyaux mtortuyaux@microsoft.com

Closes: flatcar/Flatcar#547


CI 🔵 : http://jenkins.infra.kinvolk.io:8080/job/os/job/manifest/4237/cldsv/
Kola test: flatcar/mantle#259

@tormath1 tormath1 self-assigned this Nov 24, 2021
@tormath1 tormath1 marked this pull request as ready for review November 24, 2021 14:28
@tormath1 tormath1 requested a review from a team November 24, 2021 14:28
# Since etcd/v3 we can't use both `--name` and `ETCD_NAME` at the same time.
# We parse the etcd command line options to find a `--name` if we found one,
# we unset the `ETCD_NAME` to not conflict with it.
[[ $(echo ${@} | grep "\-\-name") ]] && unset ETCD_NAME
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would miss a case when someone uses just one dash (-name) instead of two dashes (--name). But grepping for a single dashed option would also match flags like -discovery-srv-name. I'd suggest iterating the ${@} array:

for f; do
    if [[ $f =~ ^-?-name=? ]]; then
        unset ETCD_NAME
        break
    fi
done

If someone passes something like --discovery-srv-name --name where --name is the name used for DNS discovery then eh, self inflicted pain I'd say.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah right, I tend to forget flag passed with only one dash (-name) - I wanted to avoid iterating over ${@} but let's do it then. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in a following force pushed commit.

`etcd` node's name was defined by `ETCD_NAME`, from `etcd/v3` the server
can't be started with both `ETCD_NAME` and `--name` supplied.

Which leads to three cases:
* `etcd-member.service` starts without further configuration, no issue
since only `ETCD_NAME=%m` is used
* `etcd-member.service` is overrided with a CLC without `name: ` key, no
issue since only `ETCD_NAME=%m` is used
* `etcd-member.service` is overrided with a CLC with a `name: ` key,
there is an issue since in the final service we will have both
`ETCD_NAME=%m` and `--name name-from-clc`

This patch conditionally unset the `ETCD_NAME` in case `--name` is
supplied.

Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
@tormath1
Copy link
Copy Markdown
Contributor Author

@krnowak thanks for the review - comment has been addressed, CI is OK and new image has been tested with: flatcar/mantle#259

Copy link
Copy Markdown
Contributor

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Cool, let's get this merged then. :)

@tormath1 tormath1 merged commit 5a5be4e into main Nov 29, 2021
@tormath1 tormath1 deleted the tormath1/etcd branch November 29, 2021 15:59
@tormath1
Copy link
Copy Markdown
Contributor Author

cherry-picked to:

  • flatcar-3066
  • flatcar-3033

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.

Etcd Fails to Start After Upgrading to 2983.2.0

3 participants