-
Notifications
You must be signed in to change notification settings - Fork 985
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
Fixes #17292 - take otp for freeipa before save @host object in db #5542
Conversation
Do not merge! This patch has not been tested yet. Can an existing organization member please verify this patch? |
2 similar comments
Do not merge! This patch has not been tested yet. Can an existing organization member please verify this patch? |
Do not merge! This patch has not been tested yet. Can an existing organization member please verify this patch? |
Issues: #17292 |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
5896e41
to
c51dda2
Compare
ok to test |
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.
@pronix: Thanks. I left some comments. Do you mind adding a test for this as well? Thanks.
@@ -27,6 +27,7 @@ def del_realm | |||
|
|||
# Adds the host to the realm, and sets otp if we get one back | |||
def set_realm(options = {}) | |||
return self.otp if self.otp.present? && !options[:update] |
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 is this needed? Just as a gard, because the realm would be set twice otherwise (once from handle_realm_without_save
and once from queue_realm_create
)?
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.
it is also big difference when you have record in ipa and trying to update it, and when you trying to recreate token for existed record.
@@ -43,6 +43,7 @@ def queue_compute | |||
end | |||
|
|||
def queue_compute_create | |||
self.handle_realm_without_save |
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.
Isn't it enough to change the priority in queue_realm_create
so that this runs earlier than the user data template rendering?
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.
you suggest set priority for queue_realm_create
less then 1 ? because setUserData
has priority 1
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.
Well, I'd set the priority for the realm update to something lower then setUserdata
but try to avoid negative numbers.
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.
so i should shift all tasks priority to +1 and insert on first place queue_realm_create
.
correct ?
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.
I think it should be enough to just increase the compute relates tasks, but in general: Yes.
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.
ok. thanks. today will update.
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
9e5236a
to
54f827e
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.
Looks good to me, but I haven't tested this.
[test foreman] |
@timogoebel looks like jenkins error. can you please retrigger it. |
test failure unrelated 💚 |
Code looks good to me, could somebody please jump in and help with testing this? |
@timogoebel help with testing meaning writing tests? I want to participate. |
@jewgeni-prokhorenko: More like taking this patch and verifying it does not break provisioning and fixes the issue by manually testing this. |
can i help with tests ? |
Sure! |
Hi @timogoebel and @pronix,
|
hello |
Hi @pronix, self.handle_ca in compute.rb and it's working fine :D |
@pronix What's the status here? Could you please rebase onto current develop? |
hello @mmoll |
i just tried the patch with my foreman 1.20 installation and it worked, would be great if this could be merged to foreman 1.20.1 |
a rebase and added tests are still needed. |
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine. More guidelines are available in Coding Standards or on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
e4e69e1
to
78bb667
Compare
any update on this PR ? |
@theforeman/core: Could somebody review this? I would do it, but I don't have a testing environment for realm things. |
Sorry it took so long. Now, I don't have realm setup as well, but I don't think it's super important to test if the patch fixes the realm issue. What's more important IMHO is test for regressions in compute resource, as you can see this patch changes priority numbers of these and this can have huge impact. I can do that. Here is an idea - how about to clear priorities for once but forever? The problem is that we have them spread over the codebase and most of them are 1, 2, 3 numbers which makes everything pretty much "random". If you grep I suggest to create a new file module Priority
PRIORITY_CORE_DHCP_CREATE = 10
PRIORITY_CORE_DHCP_DELETE = 9
# etc for all orchestration "queue.create" calls
# Katello
PRIORITY_KATELLO_CV_CREATE = 30 This could be mixed in everywhere needed and then we would benefit from a single place with all priorities. We could do a breaking change and setup all orchestration order for once and forever so at least in core we have it in expected order and with some reasonable numbers like 100, 200, 300, 310, 320 so plugins could take advantage of that. I would file such a patch if folks are interested - @timogoebel and @tbrisker Edit: I think it's worth to actually allow plugin authors to commit changes to the file so even plugins cant take advantage of seeing everything in one place. This is not tight coupling, this is just a list of numbers on a single place. |
A simple example with libvirt VM works just fine:
I am giving this a go, we are unlikely able to test all the plugins. |
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.
Thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=1392620
For render user data with otp for freeipa registrytion, host object should have otp before created job for render userdata.