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

images switch server to centos:stream9 as base image #98

Closed
wants to merge 4 commits into from

Conversation

obnoxxx
Copy link
Collaborator

@obnoxxx obnoxxx commented Jan 18, 2023

Fixes: #78

Signed-off-by: Michael Adam obnox@samba.org

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 18, 2023

@phlogistonjohn - I wanted to take a stab at fixing an issue... building server and ad-serverstrangely fails locally for me. let's see how the ci goes ...

@obnoxxx obnoxxx requested a review from synarete January 18, 2023 18:26
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 18, 2023

ok. both server and ad-server builds fail in the CI here as well. will try to fix it later / tomorrow ...

@phlogistonjohn
Copy link
Collaborator

AFAIK, we discussed this a while back and one of the major blockers was that centos samba builds don't include the DC component at all. Thoughts?

@phlogistonjohn
Copy link
Collaborator

Just to install all the distro packages we need for the server image we'll need a bunch of commands like:

dnf --setopt=install_weak_deps=False install -y 'dnf-command(config-manager)'
dnf --setopt=install_weak_deps=False config-manager --set-enabled crb  highavailability resilientstorage
dnf --setopt=install_weak_deps=False install -y epel-release
dnf --setopt=install_weak_deps=False config-manager --set-enabled epel

In the install-packages.sh script.

However, you'll then hit the issue that sambacc fails to install because the rpm is built using fedora which is using python3.10 and when it tries to install sambacc it'll fail because the version of python is too low on cs9.

That's all on top of the issue of not having samba-dc rpm at all as mentioned earlier, so while I don't think making server cs9 based is insurmountable, there are a bunch of issues that need to be discussed IMO.

@anoopcs9
Copy link
Collaborator

@obnoxxx @phlogistonjohn

I have the required changes still on hold for quite some time(as mentioned in #78 (comment)). I've rebased those changes against latest master for both sambacc and samba-container.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 19, 2023

Updated the PR including the patch anoopcs9@075b655 from @anoopcs9 for the server image and trying to adjust for ad-server.
It failed locally for me. let's see what the ci thinks ... @anoopcs9 , @phlogistonjohn

@obnoxxx obnoxxx requested a review from anoopcs9 January 19, 2023 15:06
@anoopcs9
Copy link
Collaborator

Updated the PR including the patch anoopcs9@075b655 from @anoopcs9 for the server image and trying to adjust for ad-server. It failed locally for me. let's see what the ci thinks ... @anoopcs9 , @phlogistonjohn

You'll have to first make changes in sambacc as I mentioned earlier in #98 (comment). Only then we get an el9 build of python3-sambacc for install during server/ad-server build process.

@phlogistonjohn
Copy link
Collaborator

@anoopcs9 can I suggest that if you have changes ready for sambacc you file a PR on that repo? I have some q's and thoughts but I think a PR (even just a draft PR) is the better way to discuss it. Thanks!

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Jan 21, 2023

@phlogistonjohn , @anoopcs9 , seems I have managed to fix all the server and ad-server builds now in the ci. 😄

@phlogistonjohn
Copy link
Collaborator

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Jan 25, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link

mergify bot commented Jan 25, 2023

rebase

✅ Branch has been successfully rebased

@mergify
Copy link

mergify bot commented Feb 8, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 8, 2023

rebasing locally worked without conflicts. git magically applied @anoopcs9 's original patch to the now inappropriately named Containerfile.fedora

After a quick discussion with @phlogistonjohn , I decided to misuse this PR to generalize the multi-flvour build just introduced by opensuse builds in PR #104, ant I added commits to add explicit choices for fedora and centos builds.

example build commands are now:

  • make -f Makefile.opensuse build-client
  • make -f Makefile.fedora build-client
  • make -f Makefile.centos build-nightly-server

We should probably add explicitci actions for those ...

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 8, 2023

fwiw, the default is still fedora.
and I forgot to mention that I dropped the commit to change the ad-server image.

@anoopcs9 's original patch to switch server to centos probably needs amending to change rge default to centos as the commit claims to do ...

@phlogistonjohn - what do you think?

@obnoxxx obnoxxx force-pushed the fix-78 branch 2 times, most recently from 5bb99b0 to 2c303eb Compare February 8, 2023 20:06
@obnoxxx obnoxxx changed the title images switch server and ad-server to centos:stream9 as base image images switch server to centos:stream9 as base image Feb 8, 2023
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 8, 2023

updated the PR to fix some typos and other issus. now some checks are failing. e.g.:

Run ./tests/test-samba-ad-server-kubernetes.sh
Creating ad server deployment...
NAME              READY   UP-TO-DATE   AVAILABLE   AGE
samba-ad-server   0/1     1            0           1s
Samba ad pod is samba-ad-server-6[8](https://github.com/samba-in-kubernetes/samba-container/actions/runs/4008915832/jobs/6883730540#step:9:9)fcf68d58-blkr8
waiting for pod to be in Running state
.
NAME                               READY   STATUS   RESTARTS   AGE
samba-ad-server-68fcf68d58-blkr8   0/1     Error    0          2s

waiting for samba to become reachable
........................................................................................................................
Error: samba ad did not become reachable
Error: Process completed with exit code 1.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 8, 2023

@phlogistonjohn , @anoopcs9 , @synarete - what do you think about this?

@@ -0,0 +1,19 @@
include Makefile
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm only 50/50 on adding a file like this as it is redundant with the defaults in Makefile. If we change something we now have to change it in two places. That said, it has the benefit of being consistent with the recent changes to add opensuse support.

Makefile.centos Show resolved Hide resolved
@@ -6,7 +6,6 @@ get_custom_repo() {
url="$1"
fname="$(basename "$url")"
dest="/etc/yum.repos.d/${fname}"
dnf install --setopt=install_weak_deps=False -y curl
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we support building from either centos or fedora we should make the script work on both (or have two scripts). Right now, this change will simply cause the build to fail on fedora.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
images/server/install-sambacc.sh Show resolved Hide resolved
@obnoxxx obnoxxx force-pushed the fix-78 branch 2 times, most recently from 924a876 to 280e23f Compare February 11, 2023 19:07
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 13, 2023

@anoopcs9 , I changed your original commit in the following way:

  • split the modification of the install scripts out into an earlier commit that enablrs the explicit centos server build via Makefile.centos:
  • changed your commit to only change the main Makefile to default to centos for the server build.

Please check if you are ok with the approach in principle.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 13, 2023

@phlogistonjohn , @anoopcs9 , @synarete , @spuiuk : if desired, I could split the commits that add explicit build flavors for fedora and centos out into a separate PR (or several), since this does not match the original topic of this PR any longer ...

@phlogistonjohn
Copy link
Collaborator

I would really like to break this PR up into smaller pieces. For example, the changes to install packages script will break fedora build, but this is masked in this PR because you also change the default behavior of the makefile.

Would you be open to creating or updating this PR to only add support for centos? That would mean a new Makefile.centos{9?}, a new Containerfile.centos9, and updates to the script that work for both fedora and centos.

obnoxxx and others added 4 commits February 13, 2023 21:28
This abstracts out a few settings  a bit.
it is intended to make future additions of other build flavors easier.

Adding a new build flavor (OS) will essentially amount to adding
OS-specific containerfiles and a os-specific Makefile that includes the
main Makefile and sets the following  two override variables:
* OS_NAME
* REPO_BASE

see Makefile.opensuse for an example.

Signed-off-by: Michael Adam <obnox@samba.org>
This follows the new opensuse pattern and allows for specifically
building  fedora based images my using the new Makefile.fedora
for the server :`make -f Makefile.fedora build-server`.
for the client :`make -f Makefile.fedora build-client`.

etc

this is achieved by adding a fedora specific Makefile that changes a few
variables to explicitlyuse fedora specific containerfiles

fedora is still the default, but this prepares for later adding more
flavors.

Signed-off-by: Michael Adam <obnox@samba.org>
 server and clientcontainer images can now be builtbased on centos stream 9  cy usinf Makefile.centos:

 server:`make -f Makefile.centos build-server`.
`client:make -f Makefile.centos build-client`.

for enabling centos vuilds for more images, corresponding Containerfiles
need to be added.

Signed-off-by: Michael Adam <obnox@samba.org>

client: add a centos based client container build flavor

use: `make -f Makefile.centos build-client`

Signed-off-by: Michael Adam <obnox@samba.org>
Signed-off-by: Anoop C S <anoopcs@samba.org>
@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Feb 13, 2023

@phlogistonjohn , as discussed earlier today, I have split the generalisation part and supporting fedora and centos as explicit flavors out into PR #107 . This PR now depends on that other PR

@mergify
Copy link

mergify bot commented Feb 18, 2023

This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Apr 14, 2023

I don't know if we still want to do this. In our discussions, I had the impression, that you don't want to change the default base OS, @phlogistonjohn - right?

@phlogistonjohn
Copy link
Collaborator

Correct. It's not that I never want to switch but it should happen in a controlled phased process were we ensure we don't lose any features/abilities and we do it at a clear point in time.
This is a PR so I think we're safe to close out this version.

@obnoxxx
Copy link
Collaborator Author

obnoxxx commented Apr 19, 2023

@phlogistonjohn wrote:

Correct. It's not that I never want to switch but it should happen in a controlled phased process were we ensure we don't lose any features/abilities and we do it at a clear point in time. This is a PR so I think we're safe to close out this version.

agreed. closing ...

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

Successfully merging this pull request may close these issues.

Switch to centos:stream9 base image
3 participants