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.

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

partial Satellite 6.10 support #397

Merged
merged 1 commit into from Oct 14, 2021
Merged

Conversation

evgeni
Copy link
Member

@evgeni evgeni commented Sep 15, 2021

This change introduces partial Satellite 6.10 support.
It's partial because the following features weren't updated yet:

  • disassociation of external capsules
  • pulp data reset

The later needs a different reset mechanism due to architectural changes in
pulp3. Probably something like this, instead of all the mongodb stuff, will
work:

  • drop pulpcore db
  • create pulpcore db
  • migrate pulpcore db

@evgeni
Copy link
Member Author

evgeni commented Sep 15, 2021

I intentionally didn't add it to the README.md yet, as it's incomplete.

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Pulp Node usage also needs fixing since that's Pulp 2: https://github.com/RedHatSatellite/satellite-clone/search?q=pulp+node

@wbclark
Copy link
Collaborator

wbclark commented Sep 15, 2021

Thanks @evgeni . Would you consider getting #392 in first (and if so, should this be rebased?)

external_capsules = []
external_capsule_ids = get_info_from_hammer("--csv capsule list --search 'feature = \"Pulp Node\"'")
external_capsule_ids = get_info_from_hammer("--csv capsule list --search '(feature = \"Pulp Node\" or feature = \"Pulpcore\") and name != \"#{internal_capsule}\"'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
external_capsule_ids = get_info_from_hammer("--csv capsule list --search '(feature = \"Pulp Node\" or feature = \"Pulpcore\") and name != \"#{internal_capsule}\"'")
external_capsule_ids = get_info_from_hammer("--csv capsule list --search '(name != \"#{internal_capsule}\"'")

I think this is simpler and works just as well; downstream does not support disabling Pulp on Capsules

Copy link
Member Author

Choose a reason for hiding this comment

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

That's probably true.

One thing that bothers me: you can update the name of a capsule, and nothing will break… or should, with the code change here, it will :D

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do url !~ https://#{internal_capsule} instead, but that also doesn't feel right

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer not to build that in. The whole concept of a Smart Proxy on the same host is IMHO a Katelloism I'd love to kill. As we're looking at other architectures, we may at some point support a satellite.example.com without a Capsule and then a content.example.com which is your primary Capsule. If you start relying only on hostnames it becomes even more obscure that you really do want Smart Proxies that have content.

I think this is simpler and works just as well; downstream does not support disabling Pulp on Capsules

It's not supported today, but your suggestion makes it harder to support it tomorrow.

We should really file an RFE for Hammer to expose the capability. The API does.

@@ -24,8 +24,11 @@ def capsule_lce_args(action, capsule_id, env)
"--csv capsule content #{action}-lifecycle-environment --id #{capsule_id} --lifecycle-environment-id #{env}"
end

# the correct way is to look at hosts with the Pulpcore feature and the mirror capability
# but hammer doesn't support that
internal_capsule = ARGV[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking this might be internal_capsule = Socket.gethostname since the hostname change has already been performed when this script is run.

But your approach of relying on the ansible variable value seems very nice and consistent. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I have bad experience with Python's socket.getfqdn(), so I'll pass ;)

@evgeni
Copy link
Member Author

evgeni commented Sep 15, 2021

Thanks @evgeni . Would you consider getting #392 in first (and if so, should this be rebased?)

Yeah, sounds good.

@evgeni
Copy link
Member Author

evgeni commented Oct 1, 2021

I've dropped the capsule part from this PR, making it strictly Satellite only. This makes it:

@evgeni evgeni marked this pull request as ready for review October 1, 2021 05:46
@evgeni
Copy link
Member Author

evgeni commented Oct 1, 2021

cc @mccun934 @sthirugn

@evgeni evgeni added this to the 6.10 support milestone Oct 1, 2021
@evgeni evgeni mentioned this pull request Oct 1, 2021
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

How do we deal with that this is only partial support? Do we document this somehow?

@evgeni
Copy link
Member Author

evgeni commented Oct 1, 2021

I'd just not document 6.10 being supported at all (in the readme) yet

@ekohl
Copy link
Collaborator

ekohl commented Oct 1, 2021

The line SUPPORTED_VERSIONS could be a bit confusing. Perhaps the PR title should also reflect that it's partial?

Copy link
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

Per previous suggestion, modify the commit message to indicate this adds initial server only support for Satellite 6.10?

@evgeni evgeni changed the title Satellite 6.10 support partial Satellite 6.10 support Oct 13, 2021
This change introduces partial Satellite 6.10 support.
It's partial because the following features weren't updated yet:
* disassociation of external capsules
* pulp data reset

The later needs a different reset mechanism due to architectural changes in
pulp3. Probably something like this, instead of all the mongodb stuff, will
work:
- drop pulpcore db
- create pulpcore db
- migrate pulpcore db
@evgeni
Copy link
Member Author

evgeni commented Oct 13, 2021

@ehelms @ekohl updated commit/PR message to reflect the current state of affairs

@@ -176,7 +176,7 @@
file:
path: /var/opt/rh/rh-postgresql12/lib/pgsql/data/postgresql.conf
state: absent
when: satellite_version in ["6.8", "6.9"]
when: satellite_version in ["6.8", "6.9", "6.10"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still feel that we should make the postgresql.conf path a variable and set it appropriately, but I'd be OK with that in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I think this is something we'll have to tackle with EL8 support anyways

@evgeni evgeni merged commit 097719d into RedHatSatellite:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants