SRVOCF-1081: List functions only from the current cluster - #185
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
pmeida
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@pmeida: This pull request references SRVOCF-1081 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.1.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
43e9b22 to
651b1c4
Compare
pmeida
changed the title
|
/test e2e-aws |
| } | ||
| filtered := items[:0] | ||
| for i, item := range items { | ||
| url := repoClusterURLs[i] |
There was a problem hiding this comment.
Can you add Url normalization here? there might be mismatch with exact string equality check
There was a problem hiding this comment.
The value is stored from clusterAPIURL at creation time, so both sides share the same format/origin by construction. Normalization would therefore only be relevant if the value came from an external or user-controlled source. In this case, although the variable is named url, it should effectively be treated as a string identifier that the backend knows deterministically:
clusterApiURLcomes from the--external-api-server-urlflag, which is configured by thefunction-operatorwhen creating the deployment. The value is accessed in the same way as in our Helm template:
https://github.com/openshift/faas-console-plugin/blob/master/charts/openshift-console-plugin/templates/deployment.yaml#L2-L4
Users should not be modifying this value manually in GitHub either way.
| g.SetLimit(10) | ||
| for i, repo := range repos { | ||
| g.Go(func() error { | ||
| repoClusterURL, err := client.GetVariable(gctx, repo.Owner, repo.Name, repoVarClusterAPIURL) |
There was a problem hiding this comment.
Can you reorder the goroutine to fetch func.yaml first, and only call GetVariable for repos that will survive filtering by namespace
There was a problem hiding this comment.
Although the current approach always does both calls for every repo. There are downside to each suggestion:
- Cluster filtering is always active (unlike namespace filtering, which is optional). Deferring
GetVariablepastfunc.yamlmeans we pay the cost of fetching and parsingfunc.yamlfor every repo before we can apply the primary filter of this PR. The cost ofGetVariableshould be lower thenGetFileContent+parseFuncYaml. - There's a correctness edge case: if
func.yamlfails or is invalid, skippingGetVariablemeans cluster filtering can't run for that repo - a repo from a different cluster with a brokenfunc.yamlwould slip through instead of being excluded. - The namespace early-return optimization only pays off when a namespace is provided and is selective. When no namespace is given (which is a common case), both orderings are equivalent in terms of API calls.
Anyway I evaluated the eficiency concerns and came up with the best solution.
Efficiency gains:
Repos from other clusters only pay for one API call (GetVariable) instead of two. In a mixed-cluster environment that can cut GitHub API usage by up to 50% per excluded repo.
On the CPU side: 2 fewer sequential loops over the repo list, and all filtering decisions are made inline during the goroutine pass so the data is touched once instead of three times.
There was a problem hiding this comment.
Maybe we can do this in one loop only with Mutex? second loop in listRepoFunctions seems redundant. What about sorting by name at HandleListFunctions?
There was a problem hiding this comment.
Switching to Mutex+append would lose the original repo order (goroutines append in completion order), add lock contention with SetLimit(10), and save nothing measurable - the compaction loop is O(n) in-memory, so it is negligible.
Happy to add a name sort. Though the better UX would be by creation date so newly created functions surface at the top rather than appearing mid-list - but that needs CreatedAt exposed on the SCM client and FE changes to append on top of the list at creation time. So I think this is out of scope for this PR.
…uster Store CLUSTER_API_URL as a GitHub Actions repo variable when a function repo is created. When listing functions, read that variable for every repo and exclude repos that point to a different cluster, so each console instance only shows functions it owns. Fixes SRVOCF-1081 Signed-off-by: Pedro Almeida <pealmeid@redhat.com>
651b1c4 to
3bb1fab
Compare
|
@pmeida: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/assign @Cragsmann |
|
/retest |
3 participants
Summary
The list merges two sources: GitHub repos tagged as serverless functions (fetched via the
SCM API) and Knative Services deployed in the cluster (fetched via theKnative func lister). A repo'sKUBECONFIGsecret may target a different cluster from the one updating it, resulting in a failure, so we filter out any repo whoseCLUSTER_API_URLvariable does not match the current cluster's API server URL.CLUSTER_API_URLas a GitHub repo variable when a function repo is created, alongside the existingKUBECONFIGsecretCLUSTER_API_URLvar matches the current cluster are shown - repos created from a different cluster are excludedStoreVariable/GetVariableerror paths, rollback on failure, and all filtering scenarioscluster-filter.test.ts) verifying cross-cluster repos are hidden. Same-cluster repos are visible test is covered by existing ones.Fixes SRVOCF-1081
Checklist
docs/ARCHITECTURE.md(if there are relevant changes to our layered architecture)docs/TESTING.md(if there are relevant changes to our testing framework or setup)