salt: Store more etcd members informations in pillar (and not use not started etcd) #2099

TeddyAndrieux · 2019-12-05T20:45:50Z

Component:

'salt'

Summary:

Add all etcd member informations in the pillar retrived from the etcd3
API and use them to compute the initial-cluster value for etcd
manifest (filtering out the member with empty name)

Fixes: #2083

bert-e · 2019-12-05T20:45:53Z

Hello teddyandrieux,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2019-12-05T20:46:01Z

Conflict

A conflict has been raised during the creation of
integration branch w/2.3/bugfix/2083-all-etcd-member-info-in-pillar with contents from w/2.2/bugfix/2083-all-etcd-member-info-in-pillar
and development/2.3.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/2.3/bugfix/2083-all-etcd-member-info-in-pillar origin/development/2.3
 $ git merge origin/w/2.2/bugfix/2083-all-etcd-member-info-in-pillar
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/2.3/bugfix/2083-all-etcd-member-info-in-pillar

bert-e · 2019-12-05T20:49:36Z

Conflict

A conflict has been raised during the creation of
integration branch w/2.4/bugfix/2083-all-etcd-member-info-in-pillar with contents from w/2.3/bugfix/2083-all-etcd-member-info-in-pillar
and development/2.4.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/2.4/bugfix/2083-all-etcd-member-info-in-pillar origin/development/2.4
 $ git merge origin/w/2.3/bugfix/2083-all-etcd-member-info-in-pillar
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/2.4/bugfix/2083-all-etcd-member-info-in-pillar

bert-e · 2019-12-05T20:54:32Z

Status

Status report is not available.

bert-e · 2019-12-05T20:54:43Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge bugfix/2083-all-etcd-member-info-in-pillar into
development/2.0
w/2.1/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.1
w/2.2/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.2
w/2.3/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.3
w/2.4/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.4
w/2.5/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.5

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2019-12-05T20:54:45Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

NicolasT · 2019-12-06T07:59:22Z

salt/metalk8s/kubernetes/etcd/installed.sls


 {#- Compute the initial state according to the existing list of node. #}
-{%- set state = "existing" if etcd_endpoints else "new" %}
+{%- set state = "existing" if etcd_members else "new" %}


Not sure this is the right approach (though indeed, it's not related to this PR/issue).

In essence, what we're saying here is "If all nodes are unavailable (which causes the member list to be empty), we consider this to be a new cluster", which is definitely not a given. Actually, only during very first bootstrap this can be the case, otherwise we're disaster-recovering or so.

Worth an issue IMO.

I agree it's not a safe approach
#2102

gdemonet

Some nitpicks, but the functionality looks good to me :)

salt/_modules/metalk8s_etcd.py

salt/metalk8s/kubernetes/etcd/installed.sls

gdemonet · 2019-12-11T09:29:49Z

salt/metalk8s/kubernetes/etcd/installed.sls

+{%- do etcd_endpoints.update({node_name: endpoint}) %}
+
+{%- set etcd_initial_cluster = [] %}
+{%- for name, ep in etcd_endpoints.items() %}


Did we really need a second loop over a dict? Do we fear duplicate names?

I think it's better to not break the etcd cluster if we have one node that changed peer urls (since we only support one peer urls here)

Unfortunatly I didn't found any good method to do it without this second loop, if you have an idea do not hesitate
At the beginning just wanted to do something like that in python directly for initial-cluster

','.join('{}={}'.format(name, ip) for name, ip in etcd_endpoints.items())

But not doable in jinja without a custom filter

My point was that we're supposed to have a unique set of members (with unique peer_urls, I'd imagine), so we could have just done:

{%- set initial_cluster = [] %} {%- for member in etcd_members | selectattr('name') %} {%- do initial_cluster.append(member['name'] ~ '=' ~ member['peer_urls'][0]) %} {%- endfor %} {%- do initial_cluster.append(node_name ~ '=' ~ endpoint %}

But it's really minor, and your approach is safer against duplicate values in etcd_members, so let's leave it as is.

Add all etcd member informations in the pillar retrived from the etcd3 API and use them to compute the `initial-cluster` value for etcd manifest (filtering out the member with empty name) Fixes: #2083

bert-e · 2019-12-11T13:54:09Z

History mismatch

Merge commit #d5e80f46bc208770d8b90af51345b2d05da3f2e9 on the integration branch
w/2.1/bugfix/2083-all-etcd-member-info-in-pillar is merging a branch which is neither the current
branch bugfix/2083-all-etcd-member-info-in-pillar nor the development branch
development/2.1.

It is likely due to a rebase of the branch bugfix/2083-all-etcd-member-info-in-pillar and the
merge is not possible until all related w/* branches are deleted or updated.

Please use the reset command to have me reinitialize these branches.

TeddyAndrieux · 2019-12-11T13:54:15Z

/reset

bert-e · 2019-12-11T13:54:22Z

Reset complete

I have successfully deleted this pull request's integration branches.

bert-e · 2019-12-11T13:54:32Z

Conflict

A conflict has been raised during the creation of
integration branch w/2.3/bugfix/2083-all-etcd-member-info-in-pillar with contents from w/2.2/bugfix/2083-all-etcd-member-info-in-pillar
and development/2.3.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/2.3/bugfix/2083-all-etcd-member-info-in-pillar origin/development/2.3
 $ git merge origin/w/2.2/bugfix/2083-all-etcd-member-info-in-pillar
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/2.3/bugfix/2083-all-etcd-member-info-in-pillar

bert-e · 2019-12-11T13:57:32Z

Status

Status report is not available.

bert-e · 2019-12-11T13:57:41Z

Conflict

A conflict has been raised during the creation of
integration branch w/2.4/bugfix/2083-all-etcd-member-info-in-pillar with contents from w/2.3/bugfix/2083-all-etcd-member-info-in-pillar
and development/2.4.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 $ git fetch
 $ git checkout -B w/2.4/bugfix/2083-all-etcd-member-info-in-pillar origin/development/2.4
 $ git merge origin/w/2.3/bugfix/2083-all-etcd-member-info-in-pillar
 $ # <intense conflict resolution>
 $ git commit
 $ git push -u origin w/2.4/bugfix/2083-all-etcd-member-info-in-pillar

bert-e · 2019-12-11T13:59:30Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge bugfix/2083-all-etcd-member-info-in-pillar into
development/2.0
w/2.1/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.1
w/2.2/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.2
w/2.3/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.3
w/2.4/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.4
w/2.5/bugfix/2083-all-etcd-member-info-in-pillar will be merged into development/2.5

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

bert-e · 2019-12-11T13:59:31Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

gdemonet · 2019-12-12T19:02:39Z

salt/metalk8s/kubernetes/etcd/installed.sls

+{%- do etcd_endpoints.update({node_name: endpoint}) %}
+
+{%- set etcd_initial_cluster = [] %}
+{%- for name, ep in etcd_endpoints.items() %}


My point was that we're supposed to have a unique set of members (with unique peer_urls, I'd imagine), so we could have just done:

{%- set initial_cluster = [] %} {%- for member in etcd_members | selectattr('name') %} {%- do initial_cluster.append(member['name'] ~ '=' ~ member['peer_urls'][0]) %} {%- endfor %} {%- do initial_cluster.append(node_name ~ '=' ~ endpoint %}

But it's really minor, and your approach is safer against duplicate values in etcd_members, so let's leave it as is.

TeddyAndrieux · 2019-12-12T19:12:46Z

/approve

bert-e · 2019-12-12T19:13:16Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.0
✔️ development/2.1
✔️ development/2.2
✔️ development/2.3
✔️ development/2.4
✔️ development/2.5

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2019-12-13T08:05:51Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.0
✔️ development/2.1
✔️ development/2.2
✔️ development/2.3
✔️ development/2.4
✔️ development/2.5

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3

Please check the status of the associated issue None.

Goodbye teddyandrieux.

TeddyAndrieux requested a review from a team December 5, 2019 20:45

NicolasT reviewed Dec 6, 2019

View reviewed changes

TeddyAndrieux mentioned this pull request Dec 6, 2019

Not safe approach for etcd initial-cluster-state #2102

Closed

gdemonet previously approved these changes Dec 11, 2019

View reviewed changes

salt: Store more etcd members informations in pillar

e601ca3

Add all etcd member informations in the pillar retrived from the etcd3 API and use them to compute the `initial-cluster` value for etcd manifest (filtering out the member with empty name) Fixes: #2083

TeddyAndrieux dismissed gdemonet’s stale review via e601ca3 December 11, 2019 13:54

TeddyAndrieux force-pushed the bugfix/2083-all-etcd-member-info-in-pillar branch from d5e80f4 to e601ca3 Compare December 11, 2019 13:54

TeddyAndrieux requested review from gdemonet and NicolasT December 11, 2019 15:59

gdemonet approved these changes Dec 12, 2019

View reviewed changes

bert-e merged commit e601ca3 into development/2.0 Dec 13, 2019

bert-e deleted the bugfix/2083-all-etcd-member-info-in-pillar branch December 13, 2019 08:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

salt: Store more etcd members informations in pillar (and not use not started etcd) #2099

salt: Store more etcd members informations in pillar (and not use not started etcd) #2099

TeddyAndrieux commented Dec 5, 2019

bert-e commented Dec 5, 2019

bert-e commented Dec 5, 2019

bert-e commented Dec 5, 2019

bert-e commented Dec 5, 2019

bert-e commented Dec 5, 2019

bert-e commented Dec 5, 2019

NicolasT Dec 6, 2019

TeddyAndrieux Dec 6, 2019

gdemonet left a comment

gdemonet Dec 11, 2019

TeddyAndrieux Dec 11, 2019

gdemonet Dec 12, 2019

bert-e commented Dec 11, 2019

TeddyAndrieux commented Dec 11, 2019

bert-e commented Dec 11, 2019

bert-e commented Dec 11, 2019

bert-e commented Dec 11, 2019

bert-e commented Dec 11, 2019

bert-e commented Dec 11, 2019

bert-e commented Dec 11, 2019

gdemonet Dec 12, 2019

TeddyAndrieux commented Dec 12, 2019

bert-e commented Dec 12, 2019

bert-e commented Dec 13, 2019

salt: Store more etcd members informations in pillar (and not use not started etcd) #2099

salt: Store more etcd members informations in pillar (and not use not started etcd) #2099

Conversation

TeddyAndrieux commented Dec 5, 2019

bert-e commented Dec 5, 2019

Hello teddyandrieux,

bert-e commented Dec 5, 2019

Conflict

bert-e commented Dec 5, 2019

Conflict

bert-e commented Dec 5, 2019

Status

bert-e commented Dec 5, 2019

Integration data created

bert-e commented Dec 5, 2019

Waiting for approval

NicolasT Dec 6, 2019

Choose a reason for hiding this comment

TeddyAndrieux Dec 6, 2019

Choose a reason for hiding this comment

gdemonet left a comment

Choose a reason for hiding this comment

gdemonet Dec 11, 2019

Choose a reason for hiding this comment

TeddyAndrieux Dec 11, 2019

Choose a reason for hiding this comment

gdemonet Dec 12, 2019

Choose a reason for hiding this comment

bert-e commented Dec 11, 2019

History mismatch

TeddyAndrieux commented Dec 11, 2019

bert-e commented Dec 11, 2019

Reset complete

bert-e commented Dec 11, 2019

Conflict

bert-e commented Dec 11, 2019

Status

bert-e commented Dec 11, 2019

Conflict

bert-e commented Dec 11, 2019

Integration data created

bert-e commented Dec 11, 2019

Waiting for approval

gdemonet Dec 12, 2019

Choose a reason for hiding this comment

TeddyAndrieux commented Dec 12, 2019

bert-e commented Dec 12, 2019

In the queue

bert-e commented Dec 13, 2019