-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove init flow #135
Remove init flow #135
Conversation
Skipping CI for Draft Pull Request. |
6ac6dfd
to
ddbdc9c
Compare
ddbdc9c
to
139e935
Compare
34032ef
to
5166e15
Compare
5166e15
to
9acac16
Compare
/retest |
67025b3
to
69401dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pod security context is now different so we need to remove that or fix it in the kuttl test
d924516
to
8076033
Compare
if (err != nil || result != ctrl.Result{}) { | ||
// We can ignore RequeueAfter as we are watching the Service resource | ||
// but we have to return while waiting for the service to be exposed | ||
return ctrl.Result{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment here is not reflecting the reality. I think you are waiting for the db sync to finish before doing the deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also I think the whole if is pointless as if there is an error then we returning at L450, if there is a requeue request from the db sync then we are returning at L452. So we will arrive here with an error or a requeue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thanks!
@@ -2,25 +2,25 @@ | |||
"command": "/usr/sbin/httpd -DFOREGROUND", | |||
"config_files": [ | |||
{ | |||
"source": "/var/lib/config-data/merged/placement.conf", | |||
"source": "/var/lib/config-data/placement.conf", | |||
"dest": "/etc/placement/placement.conf", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we want to use /etc/placement/placement.conf.d/ but that can be done separately. Also at that point we want to put an empty file to /etc/placement/placement.conf as the container image put the generated sample config file there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack, should I create new issue or this will be handle as a part of different feature/issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please either create an issue or a quick follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in 826dafd
41540b8
to
33e435c
Compare
/test placement-operator-build-deploy-kuttl i think the test is racy and its not waiting long enough for it to scale we may need to merge #125 first or modify how kuttl is run placement_kuttl_run https://github.com/openstack-k8s-operators/install_yamls/blob/60c1fb7828a91545e52b5c799162bcc21c3dd5ee/Makefile#L1586-L1588 its currently looking for kuttl config in repo in https://github.com/openstack-k8s-operators/placement-operator/blob/main/kuttl-test.yaml so we can adjust some of the config that way nova is using a timeout of 300 seconds per test we are using 180 in placement so we could start by bumping to 300 and see if that is enough to stableise the test |
the secuirty context thing is fixed so i think if we dont land on a slow node this will pass
33e435c
to
18fc0ca
Compare
Thanks! I bumped it to 300 because this failed already few times on same issue that db sync didn't finished |
kuttl tests catch realll issue with volumes for db-sync so this is not related to timeout |
bb6f196
to
75540e2
Compare
/retest |
75540e2
to
0df1a65
Compare
0df1a65
to
fa32909
Compare
pkg/placement/volumes.go
Outdated
{ | ||
Name: "config-data-merged", | ||
Name: "config-data", | ||
MountPath: "/var/lib/config-data/merged", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets do a separate follow up where the merged postfix is removed from the mount path. That naming is the from the past where the init container merged configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
annotations: | ||
openshift.io/scc: anyuid | ||
labels: | ||
service: placement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we dropped the Pod assert? I guess we still have the same placement api pod running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gibizer, mrkisaolamb 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 |
4285571
into
openstack-k8s-operators:main
No description provided.