[3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd #7801

nak3 · 2018-04-05T11:16:11Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1564098

nak3 · 2018-04-05T11:17:48Z

The entire code was removed in master branch, but back port of ddf1aa2 is difficult (I think).

vrutkovs · 2018-04-05T11:25:49Z

Not sure why would we want to support embedded etcd - the first task in migrate is verifying if embedded etcd is used - and requires to migrate to external etcd, see https://github.com/openshift/openshift-ansible/blob/release-3.7/playbooks/common/openshift-etcd/migrate.yml#L11

nak3 · 2018-04-05T13:01:22Z

The customer was supposed to finish the migration to external etcd, you can refer to following logs attached in https://bugzilla.redhat.com/show_bug.cgi?id=1564098.

PLAY [Check if the master has embedded etcd] ******************************************************************************************************************************************************************

TASK [fail] ***************************************************************************************************************************************************************************************************
skipping: [localhost]

PLAY [Run pre-checks] *****************************************************************************************************************************************************************************************

But their master is still single. Do you mean that embedded2external.yml should have doen the change of separation from master to master-api/mater-controllers?

vrutkovs · 2018-04-05T16:07:06Z

Oh, correct, my fault. I was under impression that api/controller is now enforced in 3.7.

/lgtm

vrutkovs · 2018-04-05T16:07:16Z

/cherrypick release-3.6

openshift-cherrypick-robot · 2018-04-05T16:07:17Z

@vrutkovs: once the present PR merges, I will cherry-pick it on top of release-3.6 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrutkovs · 2018-04-05T16:07:36Z

/retest

nak3 · 2018-04-06T04:55:09Z

/retest

nak3 · 2018-04-06T07:46:47Z

openshift-jenkins/system-containers has an issue on 3.7 branch. I opened #7832

vrutkovs · 2018-04-06T09:17:17Z

/retest

System containers is not a blocking test AFAIK

vrutkovs · 2018-04-07T10:14:00Z

/retest

sdodson

The problem happens during the embedded to external migration which happens before upgrading to 3.7, which is before we convert everything to HA services.

I'm concerned that this same handler would be called when applying a 3.7 errata where we'd have the split sercices. I think rather than checking the number of masters we need to check if the service is enabled.

nak3 · 2018-04-08T07:06:23Z

/retest

nak3 · 2018-04-08T07:58:58Z

/retest

nak3 · 2018-04-08T14:33:34Z

That makes sense. Thank you. Updated. (Don't know why logging test started failing...)

vrutkovs · 2018-04-09T12:35:56Z

/retest

nak3 · 2018-04-09T22:09:48Z

/retest

nak3 · 2018-04-11T05:23:49Z

/retest

nak3 · 2018-04-11T05:58:47Z

ci/openshift-jenkins/logging also has an issue in 3.7 branch. These PR have same issue with this PR.

#7875
#7891
#7892

DanyC97 · 2018-04-11T09:44:15Z

@nak3 so what is the issue with #7875 & ci/openshift-jenkins/logging ?

nak3 · 2018-04-11T14:10:33Z

@DanyC97 It is same issue with openshift/origin#18184
Upstream (master branch) fixed the issue, but release-3.7 didn't fix it. I requested if it is possible to merge it into release-3.7 as openshift/origin#18647 (comment)

vrutkovs · 2018-04-30T13:38:41Z

/lgtm

sdodson · 2018-04-30T13:49:48Z

roles/openshift_master/handlers/main.yml

@@ -6,6 +6,7 @@
  when:
  - not (master_api_service_status_changed | default(false) | bool)
  - openshift.master.cluster_method == 'native'
+  - master_api_enabled.rc == 0 or openshift_version | version_compare('3.7','>=')


I wonder if there may be cases where this code is called and this variable may be undefined? Perhaps
- master_api_enabled is defined and (master_api_enabled.rc == 0 or openshift_version | version_compare('3.7','>='))

Thank you. Yes, it is safer.

I wondered if I should write these as:

- master_api_enabled is defined - master_api_enabled.rc == 0 or openshift_version | version_compare('3.7','>=')

But maybe - master_api_enabled is defined and (master_api_enabled.rc == 0 or openshift_version | version_compare('3.7','>=')) is easier to understand why master_api_enabled is defined is checked.

sdodson · 2018-04-30T13:50:24Z

roles/openshift_master/handlers/main.yml

+    name: "{{ openshift.common.service_type }}-master"
+    state: restarted
+  when:
+  - single_master_enabled.rc == 0 or openshift_version | version_compare('3.7','<')


likewise, should we check for undefined here?

openshift-ci-robot · 2018-04-30T14:04:40Z

New changes are detected. LGTM label has been removed.

openshift-ci-robot · 2018-04-30T14:04:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nak3
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vrutkovs

Assign the PR to them by writing /assign @vrutkovs in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2018-04-30T14:14:01Z

@nak3: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/gcp	`6ccdac4`	link	`/test gcp`
ci/openshift-jenkins/system-containers	`6ccdac4`	link	`/test system-containers`
ci/openshift-jenkins/logging	`6ccdac4`	link	`/test logging`
ci/openshift-jenkins/install	`6ccdac4`	link	`/test install`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vrutkovs · 2018-04-30T17:42:49Z

Verified that it works without openshift_master_cluster_method set and with openshift_master_cluster_method: native

openshift-cherrypick-robot · 2018-04-30T17:43:13Z

@vrutkovs: #7801 failed to apply on top of branch "release-3.6":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	roles/openshift_master/handlers/main.yml
A	roles/openshift_master/tasks/update_etcd_client_urls.yml
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): roles/openshift_master/tasks/update_etcd_client_urls.yml deleted in HEAD and modified in Add restart single master handler for etcdv3 upgrade from embedded etcd. Version Add restart single master handler for etcdv3 upgrade from embedded etcd of roles/openshift_master/tasks/update_etcd_client_urls.yml left in tree.
Auto-merging roles/openshift_master/handlers/main.yml
CONFLICT (content): Merge conflict in roles/openshift_master/handlers/main.yml
Patch failed at 0001 Add restart single master handler for etcdv3 upgrade from embedded etcd

In response to this:

/cherrypick release-3.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Add restart single master handler for etcdv3 upgrade from embedded etcd

2796917

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 5, 2018

nak3 changed the title ~~Add restart single master handler for etcdv3 upgrade from embedded etcd~~ [3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd Apr 5, 2018

openshift-ci-robot assigned vrutkovs Apr 5, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2018

nak3 mentioned this pull request Apr 6, 2018

test: openshift-jenkins/system-containers always fail when pushing to 3.7 branch #7832

Closed

vrutkovs mentioned this pull request Apr 7, 2018

Question - why a 3.7 hotfix branch? #7846

Closed

sdodson requested changes Apr 7, 2018

View reviewed changes

Change to checking service is enable or not for restart handler

74531d6

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 8, 2018

nak3 mentioned this pull request Apr 11, 2018

Update custom tito tagger for new version openshift/origin#18647

Merged

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2018

sdodson reviewed Apr 30, 2018

View reviewed changes

Add undefined variables check

6ccdac4

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2018

vrutkovs merged commit 99331ae into openshift:release-3.7 Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd #7801

[3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd #7801

nak3 commented Apr 5, 2018

nak3 commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

nak3 commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

openshift-cherrypick-robot commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

nak3 commented Apr 6, 2018

nak3 commented Apr 6, 2018 •

edited

Loading

vrutkovs commented Apr 6, 2018

vrutkovs commented Apr 7, 2018

sdodson left a comment

nak3 commented Apr 8, 2018

nak3 commented Apr 8, 2018

nak3 commented Apr 8, 2018

vrutkovs commented Apr 9, 2018

nak3 commented Apr 9, 2018

nak3 commented Apr 11, 2018

nak3 commented Apr 11, 2018

DanyC97 commented Apr 11, 2018

nak3 commented Apr 11, 2018

vrutkovs commented Apr 30, 2018

sdodson Apr 30, 2018

nak3 Apr 30, 2018

sdodson Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018 •

edited

Loading

vrutkovs commented Apr 30, 2018

openshift-cherrypick-robot commented Apr 30, 2018

[3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd #7801

[3.7] Add restart single master handler for etcdv3 upgrade from embedded etcd #7801

Conversation

nak3 commented Apr 5, 2018

nak3 commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

nak3 commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

openshift-cherrypick-robot commented Apr 5, 2018

vrutkovs commented Apr 5, 2018

nak3 commented Apr 6, 2018

nak3 commented Apr 6, 2018 • edited Loading

vrutkovs commented Apr 6, 2018

vrutkovs commented Apr 7, 2018

sdodson left a comment

Choose a reason for hiding this comment

nak3 commented Apr 8, 2018

nak3 commented Apr 8, 2018

nak3 commented Apr 8, 2018

vrutkovs commented Apr 9, 2018

nak3 commented Apr 9, 2018

nak3 commented Apr 11, 2018

nak3 commented Apr 11, 2018

DanyC97 commented Apr 11, 2018

nak3 commented Apr 11, 2018

vrutkovs commented Apr 30, 2018

sdodson Apr 30, 2018

Choose a reason for hiding this comment

nak3 Apr 30, 2018

Choose a reason for hiding this comment

sdodson Apr 30, 2018

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018

openshift-ci-robot commented Apr 30, 2018 • edited Loading

vrutkovs commented Apr 30, 2018

openshift-cherrypick-robot commented Apr 30, 2018

nak3 commented Apr 6, 2018 •

edited

Loading

openshift-ci-robot commented Apr 30, 2018 •

edited

Loading