Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(#351) code_manager: Switch default to undef #352

Merged
merged 1 commit into from
Oct 10, 2023
Merged

(#351) code_manager: Switch default to undef #352

merged 1 commit into from
Oct 10, 2023

Conversation

bastelfreak
Copy link
Collaborator

No description provided.

@bastelfreak bastelfreak requested review from a team as code owners May 19, 2023 09:19
@CLAassistant
Copy link

CLAassistant commented May 19, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@bastelfreak bastelfreak mentioned this pull request May 19, 2023
Closed
@bastelfreak bastelfreak force-pushed the issue_351 branch 3 times, most recently from f5e5662 to fe70fa1 Compare May 19, 2023 11:34
@bastelfreak bastelfreak force-pushed the issue_351 branch 2 times, most recently from 5a2a25a to e08c4b5 Compare August 14, 2023 11:10
Sharpie
Sharpie reviewed Aug 15, 2023
View reviewed changes
plans/subplans/install.pp
} else {
$_code_manager_auto_configure = $code_manager_auto_configure
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does installation of Larger or XL installations w/ replicas still succeed? If I recall correctly, Code Manager defaults to enabled in peadm because replica provisioning fails if the service is not running.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sharpie is correct! We need code manager for HA environments. There is already a plan for those environments which for any reasons cant use code manager divert_code_manager (https://github.com/puppetlabs/puppetlabs-peadm/blob/main/plans/misc/divert_code_manager.pp). That said, it isnt great but doable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

installation with replica and/or compilers still works. In those cases code-manager is configured/enabled

@CoMfUcIoS
Copy link
Contributor

@bastelfreak can you please have a look at those failing CI Jobs? As @Sharpie and I said, the problem is that we need/use code manager for L and XL installations.

@bastelfreak
Copy link
Collaborator Author

@CoMfUcIoS the majority of the fails are caused by a yanked? PE release. Can you take a look at #391 ?

@CoMfUcIoS
Copy link
Contributor

@CoMfUcIoS the majority of the fails are caused by a yanked? PE release. Can you take a look at #391 ?

I just merged that PR thanks for the review. Rebase and i think that Large and XL installation test will fail again because of the missing Code manager.

@bastelfreak
Copy link
Collaborator Author 

provision
  Starting: plan peadm_spec::provision_test_cluster
  Starting: task provision::provision_service on localhost
  Finished: task provision::provision_service with 1 failure in 49.94 sec
  Finished: plan peadm_spec::provision_test_cluster in 49.95 sec
  Failed on localhost:
    provision service returned an error
  Failed on 1 target: localhost
  Ran on 1 target
  Error: Process completed with exit code 1.

Is there any option to get more debug output? thats from the Upgrade test matrix / PE 2019.8.12 standard on almalinux-cloud/almalinux-8 (pull_request) job.

@CoMfUcIoS
Copy link
Contributor 

provision
  Starting: plan peadm_spec::provision_test_cluster
  Starting: task provision::provision_service on localhost
  Finished: task provision::provision_service with 1 failure in 49.94 sec
  Finished: plan peadm_spec::provision_test_cluster in 49.95 sec
  Failed on localhost:
    provision service returned an error
  Failed on 1 target: localhost
  Ran on 1 target
  Error: Process completed with exit code 1.

Is there any option to get more debug output? thats from the Upgrade test matrix / PE 2019.8.12 standard on almalinux-cloud/almalinux-8 (pull_request) job.

Those errors are from problems with our provisioning queue. I know that sometime could be temperamental, we rekick any tests that fails there. I know changes are happening atm to prevent issues like that and hopefully vmpooler will be stable again.

Feel free to re kick this test or i will as i usually do.

@bastelfreak
Copy link
Collaborator Author

@CoMfUcIoS can I get to any of the logs? The CI output isn't helpful :(

@CoMfUcIoS
Copy link
Contributor

CoMfUcIoS commented Sep 28, 2023 •
edited
Loading

@CoMfUcIoS can I get to any of the logs? The CI output isn't helpful :(

Re kicked some of the failures with DEBUG on, let's see if we get anything useful.

@CoMfUcIoS
Copy link
Contributor

@bastelfreak cant get more logs from it atm. Looks like it fails in the configure subplan where we use code manager

puppetlabs-peadm/plans/subplans/configure.pp

Line 96 in 8732a35

class { 'peadm::setup::node_manager':

@bastelfreak
Copy link
Collaborator Author

the tests are all passing now.

CoMfUcIoS
CoMfUcIoS approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@CoMfUcIoS CoMfUcIoS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ragingra ragingra merged commit d14c63f into puppetlabs:main Oct 10, 2023
51 checks passed
@bastelfreak
Copy link
Collaborator Author

thanks a lot! any chance you can do a new release soonish?

@bastelfreak bastelfreak deleted the issue_351 branch October 10, 2023 15:24
Magisus
Magisus reviewed Oct 27, 2023
View reviewed changes
plans/subplans/install.pp
# * it isn't explicitly disabled *and* the user provided r10k repo+key
# * a replica is present
# * one or multiple compiler are present
$_code_manager_auto_configure = if $r10k_private_key and $code_manager_auto_configure {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional doesn't account for the case where a user might provide just a remote and not a key, as they would if they were using an HTTPS clone URL for a public repo, which doesn't require any creds or key. Would it make sense to check for the remote to be set here instead of the key, since I think that is the minimum required for Code Manager to work properly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp, you're right! I had it the way you described it before and broke it during the latest rebase...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it in #401

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

@Magisus Magisus Magisus left review comments

@Sharpie Sharpie Sharpie left review comments

@CoMfUcIoS CoMfUcIoS CoMfUcIoS approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bastelfreak @CLAassistant @CoMfUcIoS @Sharpie @Magisus @ragingra