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

Update tekton by moving tasks to separate file #450

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

@AnnaZivkovic AnnaZivkovic changed the title update tekton by moving tasks to separate file Update tekton by moving tasks to separate file Feb 4, 2025
@AnnaZivkovic AnnaZivkovic force-pushed the update-tekton-templates branch from e692197 to a1c4c67 Compare February 4, 2025 20:41
Copy link

openshift-ci bot commented Feb 4, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from annazivkovic. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@AnnaZivkovic AnnaZivkovic force-pushed the update-tekton-templates branch 11 times, most recently from 5e27a3d to 658d76a Compare February 5, 2025 01:26
@@ -29,6 +29,12 @@ spec:
value: .
- name: revision
value: '{{revision}}'
- name: build-platforms
value:
- localhost
Copy link
Member

Choose a reason for hiding this comment

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

(unrelated to the PR)

this sounds unsafe for a future where a Konflux cluster might run on another arch than the (correctly, but also just currently) assumed amd64 or even in the multi-arch compute possibility.

cc @jeffdyoung @arewm it would be nice if konflux makes it always explicit which architecture we are requesting instead of relying on the assumption that 'localhost' (konflux cluster) is amd64.

Maybe, the script that processes this array might inspect the cluster where the builds are going to run and check whether the platform is available in the cluster (local build0 or not (delegate the build to the multi-platform manager):

node_arches = $(oc get nodes -o jsonpath="{range .items[*]}{.status.nodeInfo.architecture}{'\n'}{end}" | sort | uniq)
for platform in platforms:
  if platform in node_arches:
    build_local(...)
  else:
    build_remote_platform_manager(...)

Copy link

Choose a reason for hiding this comment

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

Yes, that is a limitation to the current implemenation. If you would prefer a targeted localhost, you can use linux/x86_64. https://github.com/redhat-appstudio/infra-deployments/blob/main/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml#L10

My desire was to change linux/amd64 to also be a local host, requiring users to explicitly specify whether they want a VM with something like linux-mlarge/amd64.

That would eventually mean that once we have multiple node pools, we might change linux/arm64 to be in-cluster, requiring users to explicitly request VMs again.

https://issues.redhat.com/browse/KONFLUX-4073 was the feature that I opened for this which has since been closed in favor of https://issues.redhat.com/browse/KONFLUX-6091.

Copy link
Member

@aleskandro aleskandro Feb 7, 2025

Choose a reason for hiding this comment

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

However, in a more k8s/ocp/nonVM-native way, that desire can be solved by allowing to set the resource requests in the Build Tasks and by configuring the cluster autoscaler with machineset autoscalers that can fit different scenarios. Once a 'large' build is created, the cluster would create a Machine with the proper size to host the build (if needed).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is a limitation to the current implemenation. If you would prefer a targeted localhost, you can use linux/x86_64. https://github.com/redhat-appstudio/infra-deployments/blob/main/components/multi-platform-controller/production/stone-prd-rh01/host-config.yaml#L10

This works and would allow us to delegate you about fixing the equivalence between localhost and amd64 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

However, in a more k8s/ocp/nonVM-native way, that desire can be solved by allowing to set the resource requests in the Build Tasks and by configuring the cluster autoscaler with machineset autoscalers

This would still not account for the case where the architecture is not supported by the platform, right?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't for now... But starting from the available arches is already a good step forward imho

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- localhost
- linux/amd64

@aleskandro
Copy link
Member

/retest

@AnnaZivkovic AnnaZivkovic force-pushed the update-tekton-templates branch from 658d76a to ad9b95b Compare February 10, 2025 22:51
@AnnaZivkovic AnnaZivkovic force-pushed the update-tekton-templates branch from ad9b95b to 1b0c3fb Compare February 13, 2025 04:58
Copy link

openshift-ci bot commented Feb 13, 2025

@AnnaZivkovic: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@aleskandro
Copy link
Member

/retest

@aleskandro aleskandro merged commit 10d4b78 into openshift:main Feb 13, 2025
11 of 15 checks passed
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.

4 participants