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

Introduce krel obs specs command to generate specs and archives for Open Build Service #2946

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

xmudrii
Copy link
Member

@xmudrii xmudrii commented Mar 6, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR is the first step for implementing KEP-1731 (kubernetes/enhancements#1731).

Introduction

This PR introduces a new subcommand in krel: krel obs specs. This command has two key responsibilities:

  • Generate RPM spec flies to be uploaded on Open Build Service
  • Create an archive with binaries that will be used by Open Build Service as a source for binaries in packages

We are intentionally introducing a new command instead of refactoring kubepkg because kubepkg is used in CI and refactoring it would break the CI.

Differences compared to kubepkg

  • krel obs specs doesn't support Debian (.deb) packages
    • Technically speaking, krel obs specs can work with Debian specs, but we are not going to maintain those specs any longer in favor of using debbuild (as described in the KEP)
  • krel obs specs cannot build packages -- we're going to use OBS for this

Workflow

The workflow is as follows:

  • krel obs specs is going to collect information about packages that we need specs and archives for
    • That includes finding the appropriate package versions, Kubernetes versions, and more
  • krel obs specs is going to generate specs based on given templates
  • krel obs specs is going to download binaries for selected packages and architectures, then archive those with the maximum compression

Example usage

# Run from the `k/release` root
krel obs specs --kubernetes-version v1.26.1

The command is going to print the output directory after finishing and you can examine that directory for correctness.

TODOs

We need to take care of the following tasks and I propose that we do that in follow up PRs:

  • Add additional tests, especially around generating specs and downloading archives
  • Documentation

Which issue(s) this PR fixes:

xref kubernetes/enhancements#1731

Does this PR introduce a user-facing change?

Introduce `krel obs specs` command to generate specs and archives for Open Build Service 

/assign @saschagrunert @cpanato @puerco
cc @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 6, 2023
@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 6, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority area/release-eng Issues or PRs related to the Release Engineering subproject labels Mar 6, 2023
@k8s-ci-robot k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 6, 2023
@xmudrii
Copy link
Member Author

xmudrii commented Mar 6, 2023

/hold
for discussion

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2023
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

So is my assumption correct that kubepkg will be used to generate the files which can be directly consumed by OBS?

If so, then I think we should build something in parallel to the existing implementation by copying the re-usable code parts and deprecating kubepkg. I think the pkg/kubepkg approach is right here, and we should avoid breaking existing CI solutions.

There are a bunch of locations where the tool, the spec files or documentation around it is already used in CI: https://cs.k8s.io/?q=kubepkg&i=nope&files=&excludeFiles=&repos=

pkg/kubepkg/fetcher.go Outdated Show resolved Hide resolved
return nil
}

func extractTarGz(gzipStream io.Reader, path string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem with this function is that it expects a file, but this function works directly with the response body without writing it to the disk. Is this a blocking comment or can I resolve it in a follow up?

Copy link
Member

Choose a reason for hiding this comment

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

We can surely follow-up on that.

@xmudrii
Copy link
Member Author

xmudrii commented Mar 7, 2023

So is my assumption correct that kubepkg will be used to generate the files which can be directly consumed by OBS?

Yes, kubepkg will generate all files and we just need to osc commit them (and wait for builds to be done).

If so, then I think we should build something in parallel to the existing implementation by copying the re-usable code parts and deprecating kubepkg.

I was thinking about that but I wasn't sure. Let's do it that way then, I'll update the PR later today.

I think the pkg/kubepkg approach is right here, and we should avoid breaking existing CI solutions.

I'm not sure I fully understand this part. Do we want to leave pkg/kubepkg package as it is, but create a new package like pkg/kubepkg2 that's going to be used for the new implementation?

@saschagrunert
Copy link
Member

So is my assumption correct that kubepkg will be used to generate the files which can be directly consumed by OBS?

Yes, kubepkg will generate all files and we just need to osc commit them (and wait for builds to be done).

We also have to osc checkout and stuff like that, but yeah I think we can have a dedicated library as part of this repository for that. The question is: do we want to use the OBS request feature to submit changes or just commit into the main repositories? For simplicity reasons we can assume that we just commit into them for now.

If so, then I think we should build something in parallel to the existing implementation by copying the re-usable code parts and deprecating kubepkg.

I was thinking about that but I wasn't sure. Let's do it that way then, I'll update the PR later today.

Thank you!

I think the pkg/kubepkg approach is right here, and we should avoid breaking existing CI solutions.

I'm not sure I fully understand this part. Do we want to leave pkg/kubepkg package as it is, but create a new package like pkg/kubepkg2 that's going to be used for the new implementation?

Yes, let's create a new package by copying the code. This way we can follow the KEP by building something in parallel. Maybe it's time to find a better name for it. I'd suggest that we use pkg/obs for everything around managing the OBS repo part, for generating the spec we could still use that namespace like pkg/obs/rpm or something?

We can then use a krel subcommand like krel obs for manual invocation.

@xmudrii
Copy link
Member Author

xmudrii commented Mar 7, 2023

We also have to osc checkout and stuff like that, but yeah I think we can have a dedicated library as part of this repository for that. The question is: do we want to use the OBS request feature to submit changes or just commit into the main repositories? For simplicity reasons we can assume that we just commit into them for now.

I was thinking about just committing them because request feature requires some manual interaction (AFAIK).

Yes, let's create a new package by copying the code. This way we can follow the KEP by building something in parallel. Maybe it's time to find a better name for it. I'd suggest that we use pkg/obs for everything around managing the OBS repo part, for generating the spec we could still use that namespace like pkg/obs/rpm or something?

We can then use a krel subcommand like krel obs for manual invocation.

Sounds like a good plan to me!

@xmudrii xmudrii changed the title Refactor kubepkg and RPM specs for kubepkg to support the OBS workflow Introduce krel obs specs command to generate specs and archives for Open Build Service Mar 7, 2023
@xmudrii
Copy link
Member Author

xmudrii commented Mar 7, 2023

@saschagrunert Ready for another round of reviews.

@xmudrii
Copy link
Member Author

xmudrii commented Mar 9, 2023

/test pull-release-test-canary

4 similar comments
@xmudrii
Copy link
Member Author

xmudrii commented Mar 9, 2023

/test pull-release-test-canary

@xmudrii
Copy link
Member Author

xmudrii commented Mar 9, 2023

/test pull-release-test-canary

@xmudrii
Copy link
Member Author

xmudrii commented Mar 9, 2023

/test pull-release-test-canary

@xmudrii
Copy link
Member Author

xmudrii commented Mar 9, 2023

/test pull-release-test-canary

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM, I just have a few things but let's iterate to be faster.

Thank you!

cmd/krel/cmd/obs.go Outdated Show resolved Hide resolved
cmd/krel/cmd/obs_specs.go Outdated Show resolved Hide resolved
pkg/obs/specs/specs.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2023
@saschagrunert
Copy link
Member

/hold
for discussion

Feel free to lift when you're ready.

Signed-off-by: Marko Mudrinić <[email protected]>
Signed-off-by: Marko Mudrinić <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2023
@xmudrii
Copy link
Member Author

xmudrii commented Mar 16, 2023

@saschagrunert Comments addressed, PTAL again.

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold for @saschagrunert

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2023
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, saschagrunert, xmudrii

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 200170a into kubernetes:master Mar 16, 2023
@xmudrii xmudrii deleted the kubepkg-archive branch March 16, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants