New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

daemon: fix taint issues #290

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Jan 11, 2019

Based on comment from previous PR:

  • fix typos and add docs
  • de-dupe taint appends
  • change file location for taint

There is still the problem of:

  • How to make the taint key more descriptive (it is currently rhcosSsh)
  • that this is breaking tests (will open in further discussions)

cc @abhinavdahiya @ashcrow

yuqi-zhang added some commits Jan 11, 2019

daemon: add doc for SSH taint functionality
Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
daemon: fixes to ssh taint
Fix typos, change file location, and de-dupe taints.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
@openshift-ci-robot

This comment has been minimized.

Copy link

openshift-ci-robot commented Jan 11, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @crawford 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:

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 openshift-ci-robot requested review from jlebon and wking Jan 11, 2019

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Jan 11, 2019

Note: Tests should be transitioned off of ssh. Some are still using ssh, but we'll likely want to work with them to make sure it changes before we enable the tainting in the OS layer.

@abhinavdahiya

This comment has been minimized.

Copy link
Member

abhinavdahiya commented Jan 11, 2019

* How to make the taint key more descriptive (it is currently `rhcosSsh`)

WDYT about:

  • if this key is supposed to be used to coordinate between components lets use machineconfiguration.openshift.io/<'-' separated name>
  • if this key is supposed to be used by only one component then lets use <component>.machineconfiguration.openshift.io/<'-' separated name>

i think we already kinda follow this..

@ashcrow

This comment has been minimized.

Copy link
Member

ashcrow commented Jan 11, 2019

I'm OK with @abhinavdahiya's naming propsal. This will cause us to need to update documentation with @chrisnegus and update the taint script, but I think that's fine.

@kikisdeliveryservice
Copy link
Member

kikisdeliveryservice left a comment

I think in a future PR we might want to add more of the why and details on the what happens when nodes are tainted.

@@ -128,3 +130,8 @@ The draining of pods on the only master node will not evict the control plane as
### Node drain etcd static pods on masters

Etcd is co-located on master nodes as static pods. The draining behavior defined above prevents draining of static pods to prevent interference to etcd cluster by the daemon.

## Tainting
The MachineConfigDaemon is also responsible for tainting the node every time an SSH access is detected. It does this via. watching a file at `/etc/machine-config-daemon/ssh-tainted`, which other scripts can write to to notify the MCD to apply the taint.

This comment has been minimized.

@kikisdeliveryservice

kikisdeliveryservice Jan 11, 2019

Member

typo -> "to to"

@@ -128,3 +130,8 @@ The draining of pods on the only master node will not evict the control plane as
### Node drain etcd static pods on masters

Etcd is co-located on master nodes as static pods. The draining behavior defined above prevents draining of static pods to prevent interference to etcd cluster by the daemon.

## Tainting
The MachineConfigDaemon is also responsible for tainting the node every time an SSH access is detected. It does this via. watching a file at `/etc/machine-config-daemon/ssh-tainted`, which other scripts can write to to notify the MCD to apply the taint.

This comment has been minimized.

@kikisdeliveryservice

kikisdeliveryservice Jan 11, 2019

Member

"via." -> "via"

## Tainting
The MachineConfigDaemon is also responsible for tainting the node every time an SSH access is detected. It does this via. watching a file at `/etc/machine-config-daemon/ssh-tainted`, which other scripts can write to to notify the MCD to apply the taint.

The taint can be removed via. `kubectl taint nodes <node> rhcosSsh-`.

This comment has been minimized.

@kikisdeliveryservice

kikisdeliveryservice Jan 11, 2019

Member

"via." -> "via"

@@ -14,6 +14,8 @@

MachineConfigDaemon is scheduled on the machines in a cluster as a DaemonSet. This daemon is responsible for performing machine updates in OpenShift 4. The update will include tasks related to the systemd units, files on disk, operating system upgrades etc. The MachineConfigDaemon updates a machine to configuration defined by MachineConfig as instructed by the MachineConfigController.

The MachineConfigDaemon is also responsible for tainting a machine with rhcosSsh=accessed:NoSchedule when it detects an SSH access to the machine.

This comment has been minimized.

@kikisdeliveryservice

kikisdeliveryservice Jan 11, 2019

Member

could we add the quotes things like this: rhcosSsh=accessed:NoSchedule

@kikisdeliveryservice
Copy link
Member

kikisdeliveryservice left a comment

Some doc changes.

@yuqi-zhang

This comment has been minimized.

Copy link
Contributor

yuqi-zhang commented Jan 11, 2019

Will fixup PR pending maybe a rebase onto: #291

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