On-cluster build: Configurable PVC size used by a Pipeline

[lance]

No description provided.

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[zroubalik]

/remove-lifecycle stale

[ocpvkb]

👍

[ocpvkb]

/remove-lifecycle stale

[tvand]

Would it be feasible to generate a YAML file that contains all the pipeline specifications initially and then use that, if it exists? That way, users can fix any issues they may have directly in the file. WDYT?

[zroubalik]

@tvand I think this is definitely feasible. Would you mind openin a separate issue for this?

[lance]

@zroubalik I think there are a couple of issues already open around this idea in a general sense. There is #1015 which is a generic request for yaml output for anything that func produces, and there is this #898 which is about refactoring func.yaml and using it to generate resource yaml. I think that 1015 is actually a good place to track this all as a whole, and I will follow up there.

[zroubalik]

@lance you are right! Thanks for poiting this out

[grafvonb]

@lance Is it still an open issue that should be implemented?

Currently the PVC has a fixed size of 5Mi defined in pipelines/tekton/defaults.go as follows:

const (
	// DefaultPersistentVolumeClaimSize represents default size of PVC created for a Pipeline,
	// specified in bytes (eg. 5Mi = 5MiB = 5 * 1024 * 1024)
	DefaultPersistentVolumeClaimSize int64 = 5 * 1024 * 1024
)

[lance]

@grafvonb yes, this is still something we'd like to implement, although I haven't really given a lot of thought to how this should surface for the user. It could be a flag on the func deploy --remote command, possibly. E.g.

func deploy --remote --git-url <url> --pvc-size 10MiB

But I am open to consideration for other options.

[grafvonb]

@lance Is there a sketch for handling configuration options? I think @lkingland mentioned it somewhere.

I know we use cobra/viper, but we also have func.yaml, so maybe it would make sense to extend viper (https://github.com/spf13/viper/blob/master/README.md#reading-config-from-ioreader) and add a new source like func.yaml? Then we get a full support for all kind of configuration sources that the user could use if needed...

[lkingland]

I looked into utilizing the cobra/viper package's inbuilt config handling, but it ended up not quite being a good fit. In short, our func.yaml is more than a config file. Take one example: We have a chicken-and-egg problem because func.yaml is the current serialized state of a function, and as such it needs to be read in (along with potentially a path flag) prior to flags being defined, but after the global config is applied. And on a more subjective note, trying to use Cobra/Viper simply felt like round-peg-square-hole.

Please see how the build command makes use of the simple config package, and the open PR which adds this same methodology to the deploy command. You will see a lot of complexity evaporates (a good indication its on the right path).

For this scenario, we would simply add a flag to the build and deploy commands, set its default value to the associated member of the Global Config, and add it to the two mapping functions Apply and Configure such that it picks up function context (state). I intend to write up how this works in an ARCHITECTURE.md when it's set up on the other commands, but I am sure if you trace through one example sstting, such as --builder, you'll see how it plugs in!

The whole global config with local function context system ended up being actually quite straightforward, and importantly command help text shows the actual value which will be used for the flag if not provided by the user, taking into account the function's current state.

[ocpvkb]

I would like to look at the topic from a different angle.
In our environment (Openshift Serverless) the func.yaml is integrated directly without any configuration possibility. As a storage provider we use NetApp with NFS to provide PVC's. The primary problem is not the fixed configuration. It is the size of 5MB. The file system and the symlink structure require a minimum size of 20MB. This could be the same situation by other storage providers... PureStore,.. tbc.
I think if the fixed size is increased to e.g. 256MB in a first iterative step, then the situation is more relaxed..

[grafvonb]

@lance This functionality is already implemented, merged using #1598 and released, so we should close this issue (however, the release notes don't contain any information about this change at the moment...).

[lance]

@lance This functionality is already implemented, merged using #1598 and released, so we should close this issue (however, the release notes don't contain any information about this change at the moment...).

Thanks for pointing this out. The release notes are generated automatically from the PRs when there is included a code section with the release-notes tag as shown in the PR template. E.g.

```release-notes
* Something about the PR here
* ```

I do try to remember to edit the PR descriptions which don't contain it, but often forget.