Skip to content

Restart MicroShift when IP address changes#650

Merged
oglok merged 3 commits into
mainfrom
ipnew
Apr 20, 2022
Merged

Restart MicroShift when IP address changes#650
oglok merged 3 commits into
mainfrom
ipnew

Conversation

@oglok

@oglok oglok commented Apr 7, 2022

Copy link
Copy Markdown
Contributor

MicroShift endpoints rely on the host IP address. If that IP changes for whatever reason, APIs will be completely unavailable and MicroShift will stop working. This PR will detect IP host changes, and shutdown MicroShift. Since the production deployment model is based on systemd service, MicroShift service will be automatically restarted and endpoints will be recreated to be back in a functional state.

Signed-off-by: Miguel Angel Ajo majopela@redhat.com
Signed-off-by: Ricardo Noriega rnoriega@redhat.com

Which issue(s) this PR addresses:

Closes #556

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2022
@openshift-ci

openshift-ci Bot commented Apr 7, 2022

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci Bot requested review from cooktheryan and mangelajo April 7, 2022 16:56
@oglok oglok changed the title WIP: Restart MicroShift when IP address changes Restart MicroShift when IP address changes Apr 8, 2022
@oglok oglok marked this pull request as ready for review April 8, 2022 11:29
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2022
@openshift-ci openshift-ci Bot requested review from copejon and husky-parul April 8, 2022 11:29
when IP address changes have been detected.

Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>
@oglok

oglok commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

To show some testing. After the IP address changes, it is detected and MicroShift exits:

W0408 13:17:35.906386  595827 ipwatch.go:58] IP address has changed from "192.168.0.140" to "192.168.0.99", restarting MicroShift                                                

Systemd will restart MicroShift service, and endpoints will be recreated:

I0408 13:22:29.958411  599949 apiservice.go:86] deleting outdated endpoints openshift-apiserver
I0408 13:22:29.997946  599949 apiservice.go:86] deleting outdated endpoints openshift-oauth-apiserver

Comment thread pkg/controllers/apiservice.go Outdated

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.

^checker complains;, the port is missing in the args?

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.

Fixed! Thx

Comment thread pkg/controllers/apiservice.go Outdated

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.

why logrus? no idea if it was originally in my patch, could be: bad Ajo! X'D

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.

My bad too, the original PR was before the klog replacement.

@mangelajo mangelajo left a comment

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.

some comments, thanks for handling it

Signed-off-by: Miguel Angel Ajo <majopela@redhat.com>
Co-authored-by: Ricardo Noriega <rnoriega@redhat.com>
@oglok

oglok commented Apr 11, 2022

Copy link
Copy Markdown
Contributor Author

/retest

@copejon

copejon commented Apr 11, 2022

Copy link
Copy Markdown
Contributor

/test e2e-rpm-install

1 similar comment
@copejon

copejon commented Apr 11, 2022

Copy link
Copy Markdown
Contributor

/test e2e-rpm-install

@oglok

oglok commented Apr 11, 2022

Copy link
Copy Markdown
Contributor Author

/retest

@mangelajo

mangelajo commented Apr 11, 2022 via email

Copy link
Copy Markdown
Contributor

@mangelajo

Copy link
Copy Markdown
Contributor

/retest

@mangelajo mangelajo self-requested a review April 12, 2022 13:45
@mangelajo

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 12, 2022
@mangelajo

Copy link
Copy Markdown
Contributor

Oh, it's not CI, something is broken:

[root@fedora ~]# oc get endpoints -A
NAMESPACE           NAME                      ENDPOINTS                                      AGE
default             kubernetes                192.168.1.70:6443                              2m12s
openshift-dns       dns-default               10.42.0.4:5353,10.42.0.4:9154,10.42.0.4:5353   83s
openshift-ingress   router-external-default                                                  83s
openshift-ingress   router-internal-default                                                  83s
[root@fedora ~]# oc get pods -A
NAMESPACE                       NAME                                  READY   STATUS    RESTARTS   AGE
kube-system                     kube-flannel-ds-s7tbr                 1/1     Running   0          5m16s
kubevirt-hostpath-provisioner   kubevirt-hostpath-provisioner-xqsg7   1/1     Running   0          5m6s
openshift-dns                   dns-default-n4kl9                     2/2     Running   0          5m16s
openshift-dns                   node-resolver-srpnj                   1/1     Running   0          5m16s
openshift-ingress               router-default-85bcfdd948-nprmj       0/1     Running   2          5m21s
openshift-service-ca            service-ca-7764c85869-nnv2j           1/1     Running   0          5m21s

Comment thread pkg/controllers/apiservice.go Outdated
@mangelajo

Copy link
Copy Markdown
Contributor

gogogo %)

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2022
@oglok

oglok commented Apr 20, 2022

Copy link
Copy Markdown
Contributor Author

There was more subtleness into the code than I expected xD xD

DeepEqual condition was not working properly, so I decided to change a bit the logic. Check if that endpoints name exist, do a proper deepEqual check and return if it exists, otherwise, create the endpoints.

Tests are ok locally, let's see CI.

Signed-off-by: Ricardo Noriega <rnoriega@redhat.com>

@mangelajo mangelajo left a comment

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.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2022
@openshift-ci

openshift-ci Bot commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mangelajo

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@oglok oglok merged commit 48863ff into main Apr 20, 2022
@oglok oglok deleted the ipnew branch April 20, 2022 13:52
@mangelajo mangelajo mentioned this pull request Jun 10, 2022
@ggiguash

ggiguash commented Jul 4, 2022

Copy link
Copy Markdown
Contributor

USHIFT-37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] If the interface IP changes Microshift stops working

4 participants