diff --git a/cmd/build.go b/cmd/build.go index ad2cc314b7..bcc4376035 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -226,6 +226,11 @@ For more options, run 'func build --help'`, err) if err != nil { return } + + if err = client.Scaffold(cmd.Context(), f, ""); err != nil { + return + } + if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { return } @@ -479,6 +484,7 @@ func (c buildConfig) clientOptions() ([]fn.Option, error) { switch c.Builder { case builders.Host: o = append(o, + fn.WithScaffolder(oci.NewScaffolder(c.Verbose)), fn.WithBuilder(oci.NewBuilder(builders.Host, c.Verbose)), fn.WithPusher(oci.NewPusher(c.RegistryInsecure, false, c.Verbose, oci.WithTransport(newTransport(c.RegistryInsecure)), @@ -487,6 +493,7 @@ func (c buildConfig) clientOptions() ([]fn.Option, error) { ) case builders.Pack: o = append(o, + fn.WithScaffolder(buildpacks.NewScaffolder(c.Verbose)), fn.WithBuilder(buildpacks.NewBuilder( buildpacks.WithName(builders.Pack), buildpacks.WithTimestamp(c.WithTimestamp), @@ -497,6 +504,7 @@ func (c buildConfig) clientOptions() ([]fn.Option, error) { docker.WithVerbose(c.Verbose)))) case builders.S2I: o = append(o, + fn.WithScaffolder(s2i.NewScaffolder(c.Verbose)), fn.WithBuilder(s2i.NewBuilder( s2i.WithName(builders.S2I), s2i.WithVerbose(c.Verbose))), diff --git a/cmd/client.go b/cmd/client.go index 57e81daaf4..9dea30f629 100644 --- a/cmd/client.go +++ b/cmd/client.go @@ -66,6 +66,7 @@ func NewClient(cfg ClientConfig, options ...fn.Option) (*fn.Client, func()) { fn.WithVerbose(cfg.Verbose), fn.WithTransport(t), fn.WithRepositoriesPath(config.RepositoriesPath()), + fn.WithScaffolder(buildpacks.NewScaffolder(cfg.Verbose)), fn.WithBuilder(buildpacks.NewBuilder(buildpacks.WithVerbose(cfg.Verbose))), fn.WithRemovers(knative.NewRemover(cfg.Verbose), k8s.NewRemover(cfg.Verbose)), fn.WithDescribers(knative.NewDescriber(cfg.Verbose), k8s.NewDescriber(cfg.Verbose)), diff --git a/cmd/deploy.go b/cmd/deploy.go index c4bc66d336..25fbc335b7 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -458,12 +458,7 @@ For more options, run 'func deploy --help'`, err) if buildOptions, err = cfg.buildOptions(); err != nil { return } - - var ( - digested bool - justBuilt bool - justPushed bool - ) + var digested bool // Validate the image and check whether its digested or not if cfg.Image != "" { @@ -483,20 +478,30 @@ For more options, run 'func deploy --help'`, err) if digested { f.Deploy.Image = cfg.Image } else { - // NOT digested, build & push the Function unless specified otherwise - if f, justBuilt, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil { + // NOT digested: scaffold, build & push as per config + var ( + shouldBuild bool + justPushed bool + ) + if shouldBuild, err = build(cmd, cfg.Build, f); err != nil { return } + if shouldBuild { + if err = client.Scaffold(cmd.Context(), f, ""); err != nil { + return + } + if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { + return + } + } if cfg.Push { if f, justPushed, err = client.Push(cmd.Context(), f); err != nil { return } } - // TODO: gauron99 - temporary fix for undigested image direct deploy - // (w/out build) This might be more complex to do than leaving like this - // image digests are created via the registry on push. - if (justBuilt || justPushed) && f.Build.Image != "" { - // f.Build.Image is set in Push for now, just set it as a deployed image + // image was just build and pushed to registry => potentially new image + if (shouldBuild || justPushed) && f.Build.Image != "" { + // f.Build.Image is set when pushed to registry, just set it as a deployed image f.Deploy.Image = f.Build.Image } } @@ -523,33 +528,24 @@ For more options, run 'func deploy --help'`, err) return f.Stamp() } -// build when flag == 'auto' and the function is out-of-date, or when the -// flag value is explicitly truthy such as 'true' or '1'. Error if flag -// is neither 'auto' nor parseable as a boolean. Return CLI-specific error -// message verbeage suitable for both Deploy and Run commands which feature an -// optional build step. Boolean return value signifies if the image has gone -// through a build process. -func build(cmd *cobra.Command, flag string, f fn.Function, client *fn.Client, buildOptions []fn.BuildOption) (fn.Function, bool, error) { - var err error +// build determines IF function should be built in current run returning an +// error, if it occurs, during flag parsing +func build(cmd *cobra.Command, flag string, f fn.Function) (bool, error) { if flag == "auto" { if f.Built() { fmt.Fprintln(cmd.OutOrStdout(), "function up-to-date. Force rebuild with --build") - return f, false, nil + return false, nil } else { - if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { - return f, false, err - } + return true, nil } } else if build, _ := strconv.ParseBool(flag); build { - if f, err = client.Build(cmd.Context(), f, buildOptions...); err != nil { - return f, false, err - } - } else if _, err = strconv.ParseBool(flag); err != nil { - return f, false, fmt.Errorf("invalid value for the build flag (%q), valid value is either 'auto' or a boolean", flag) + return true, nil + } else if _, err := strconv.ParseBool(flag); err != nil { + return false, fmt.Errorf("invalid value for the build flag (%q), valid value is either 'auto' or a boolean", flag) } else if !build { - return f, false, nil + return false, nil } - return f, true, nil + return false, nil } func NewRegistryValidator(path string) survey.Validator { diff --git a/cmd/func-util/main.go b/cmd/func-util/main.go index fdaa31a132..8edd03379d 100644 --- a/cmd/func-util/main.go +++ b/cmd/func-util/main.go @@ -18,6 +18,7 @@ import ( "github.com/openshift/source-to-image/pkg/cmd/cli" "k8s.io/klog/v2" + "knative.dev/func/pkg/buildpacks" fn "knative.dev/func/pkg/functions" "knative.dev/func/pkg/k8s" "knative.dev/func/pkg/knative" @@ -74,12 +75,12 @@ func socat(ctx context.Context) error { } func scaffold(ctx context.Context) error { - - if len(os.Args) != 2 { - return fmt.Errorf("expected exactly one positional argument (function project path)") + if len(os.Args) != 3 { + return fmt.Errorf("expected exactly 2 positional argument (function project path & builder)") } path := os.Args[1] + builder := os.Args[2] f, err := fn.NewFunction(path) if err != nil { @@ -100,37 +101,30 @@ func scaffold(ctx context.Context) error { return fmt.Errorf("cannot write middleware version as a result: %w", err) } - if f.Runtime != "go" && f.Runtime != "python" { - // Scaffolding is for now supported/needed only for Go/Python. - return nil - } - - appRoot := filepath.Join(f.Root, ".s2i", "builds", "last") - _ = os.RemoveAll(appRoot) - - err = scaffolding.Write(appRoot, f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()) - if err != nil { - return fmt.Errorf("cannot write the scaffolding: %w", err) - } - - if err := os.MkdirAll(filepath.Join(f.Root, ".s2i", "bin"), 0755); err != nil { - return fmt.Errorf("unable to create .s2i bin dir. %w", err) - } - - var asm string - switch f.Runtime { - case "go": - asm = s2i.GoAssembler - case "python": - asm = s2i.PythonAssembler + switch builder { + case "pack": + appRoot := filepath.Join(f.Root, ".func", "build") + _ = os.RemoveAll(appRoot) + client := fn.New( + fn.WithScaffolder(buildpacks.NewScaffolder(true)), + ) + err = client.Scaffold(ctx, f, appRoot) + if err != nil { + return err + } + case "s2i": + appRoot := filepath.Join(f.Root, ".s2i", "build") + _ = os.RemoveAll(appRoot) + client := fn.New( + fn.WithScaffolder(s2i.NewScaffolder(true)), + ) + err = client.Scaffold(ctx, f, appRoot) + if err != nil { + return err + } default: - panic("unreachable") + return fmt.Errorf("unknown builder in func-util image '%v'", builder) } - - if err := os.WriteFile(filepath.Join(f.Root, ".s2i", "bin", "assemble"), []byte(asm), 0755); err != nil { - return fmt.Errorf("unable to write go assembler. %w", err) - } - return nil } @@ -143,10 +137,6 @@ func s2iCmd(ctx context.Context) error { func deploy(ctx context.Context) error { var err error - deployer := knative.NewDeployer( - knative.WithDeployerVerbose(true), - knative.WithDeployerDecorator(deployDecorator{})) - var root string if len(os.Args) > 1 { root = os.Args[1] @@ -168,7 +158,14 @@ func deploy(ctx context.Context) error { f.Deploy.Image = f.Image } - res, err := deployer.Deploy(ctx, f) + client := fn.New( + fn.WithDeployer( + knative.NewDeployer( + knative.WithDeployerDecorator(deployDecorator{}), + knative.WithDeployerVerbose(true)), + ), + ) + res, err := client.Deploy(ctx, f, fn.WithDeploySkipBuildCheck(true)) if err != nil { return fmt.Errorf("cannot deploy the function: %w", err) } diff --git a/cmd/run.go b/cmd/run.go index 5f4ec65036..35d9382402 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -227,9 +227,19 @@ Or if you have an existing function: // actual build step if !digested { - if f, _, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil { + shouldBuild, err := build(cmd, cfg.Build, f) + if err != nil { return err } + if shouldBuild { + if err := client.Scaffold(cmd.Context(), f, ""); err != nil { + return err + } + f, err = client.Build(cmd.Context(), f, buildOptions...) + if err != nil { + return err + } + } } } else { // if !container // don't run digested image without a container diff --git a/pkg/builders/builders.go b/pkg/builders/builders.go index 81e2c13cbd..ec96ee21d7 100644 --- a/pkg/builders/builders.go +++ b/pkg/builders/builders.go @@ -40,6 +40,11 @@ func (k Known) String() string { return b.String() } +type BuildLock interface { + Unlock() + Lock() +} + // ErrUnknownBuilder may be used by whomever is choosing a concrete // implementation of a builder to invoke based on potentially invalid input. type ErrUnknownBuilder struct { diff --git a/pkg/builders/builders_int_test.go b/pkg/builders/builders_int_test.go index c6a9e9b815..129ba14dba 100644 --- a/pkg/builders/builders_int_test.go +++ b/pkg/builders/builders_int_test.go @@ -139,6 +139,7 @@ password=nbusr123 testCases := []struct { Name string + Scaffolder fn.Scaffolder Builder fn.Builder BuilderImage func(ctx context.Context, t *testing.T, certDir string) string Envs []fn.Env @@ -146,6 +147,7 @@ password=nbusr123 }{ { Name: "s2i", + Scaffolder: s2i.NewScaffolder(true), Builder: s2i.NewBuilder(s2i.WithVerbose(true)), BuilderImage: buildPatchedS2IBuilder, Mounts: []fn.MountSpec{ @@ -156,6 +158,7 @@ password=nbusr123 }, { Name: "pack", + Scaffolder: buildpacks.NewScaffolder(true), Builder: buildpacks.NewBuilder(buildpacks.WithVerbose(true)), BuilderImage: buildPatchedBuildpackBuilder, Envs: []fn.Env{ @@ -184,6 +187,10 @@ password=nbusr123 f.Build.Mounts = append(f.Build.Mounts, tt.Mounts...) f.Build.BuildEnvs = append(f.Build.BuildEnvs, tt.Envs...) + err = tt.Scaffolder.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } err = tt.Builder.Build(ctx, f, nil) if err != nil { t.Fatal(err) @@ -282,7 +289,7 @@ USER 1001:0 // Builds a tiny paketo builder that trusts to our self-signed certificate (see createCertificate). func buildPatchedBuildpackBuilder(ctx context.Context, t *testing.T, certDir string) string { tag := "localhost:50000/builder-jammy-tin:test" - dockerfile := `FROM ghcr.io/knative/builder-jammy-tiny:latest + dockerfile := `FROM ghcr.io/knative/builder-jammy-tiny:v2 COPY 85c05568.0 /etc/ssl/certs/ ` return buildPatchedBuilder(ctx, t, tag, dockerfile, certDir) diff --git a/pkg/buildpacks/builder.go b/pkg/buildpacks/builder.go index a341b08226..309d612b70 100644 --- a/pkg/buildpacks/builder.go +++ b/pkg/buildpacks/builder.go @@ -28,8 +28,8 @@ import ( // DefaultName when no WithName option is provided to NewBuilder const DefaultName = builders.Pack -var DefaultBaseBuilder = "ghcr.io/knative/builder-jammy-base:latest" -var DefaultTinyBuilder = "ghcr.io/knative/builder-jammy-tiny:latest" +var DefaultBaseBuilder = "ghcr.io/knative/builder-jammy-base:v2" +var DefaultTinyBuilder = "ghcr.io/knative/builder-jammy-tiny:v2" var ( DefaultBuilderImages = map[string]string{ @@ -186,6 +186,11 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf opts.Env["BPE_DEFAULT_LISTEN_ADDRESS"] = "[::]:8080" } + // set scaffolding path to buildpacks builder + if f.Runtime == "go" { + opts.Env["BP_GO_WORKDIR"] = ".func/build" + } + var bindings = make([]string, 0, len(f.Build.Mounts)) for _, m := range f.Build.Mounts { bindings = append(bindings, fmt.Sprintf("%s:%s", m.Source, m.Destination)) diff --git a/pkg/buildpacks/scaffolder.go b/pkg/buildpacks/scaffolder.go new file mode 100644 index 0000000000..ed116b1c12 --- /dev/null +++ b/pkg/buildpacks/scaffolder.go @@ -0,0 +1,50 @@ +package buildpacks + +import ( + "context" + "fmt" + "os" + "path/filepath" + + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/scaffolding" +) + +const defaultPath = ".func/build" + +// Scaffolder will scaffold Function for Buildpacks +type Scaffolder struct { + verbose bool +} + +func NewScaffolder(verbose bool) *Scaffolder { + return &Scaffolder{verbose: verbose} +} + +// Scaffold the function so that it can be built via buildpacks builder. +// 'path' is an optional override. Assign "" (empty string) most of the time +func (s Scaffolder) Scaffold(ctx context.Context, f fn.Function, path string) error { + // Because of Python injector we currently dont scaffold python. + // Add it here once the injector is removed + if f.Runtime != "go" { + if s.verbose { + fmt.Println("Scaffolding skipped. Currently available for runtime go") + } + return nil + } + + appRoot := path + if appRoot == "" { + appRoot = filepath.Join(f.Root, defaultPath) + } + if s.verbose { + fmt.Printf("Writing buildpacks scaffolding at path '%v'\n", appRoot) + } + + embeddedRepo, err := fn.NewRepository("", "") + if err != nil { + return fmt.Errorf("unable to load embedded scaffolding: %w", err) + } + _ = os.RemoveAll(appRoot) + return scaffolding.Write(appRoot, f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()) +} diff --git a/pkg/deployer/testing/integration_test_helper.go b/pkg/deployer/testing/integration_test_helper.go index 483ca0a130..b3bae454ca 100644 --- a/pkg/deployer/testing/integration_test_helper.go +++ b/pkg/deployer/testing/integration_test_helper.go @@ -42,6 +42,7 @@ func TestInt_Deploy(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -65,6 +66,12 @@ func TestInt_Deploy(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc t.Fatal(err) } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { @@ -115,6 +122,7 @@ func TestInt_Metadata(t *testing.T, deployer fn.Deployer, remover fn.Remover, de t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -156,6 +164,7 @@ func TestInt_Metadata(t *testing.T, deployer fn.Deployer, remover fn.Remover, de t.Fatal(err) } handlerPath := filepath.Join(root, "handle.go") + if err := os.WriteFile(handlerPath, []byte(testHandler), 0644); err != nil { t.Fatal(err) } @@ -197,6 +206,12 @@ func TestInt_Metadata(t *testing.T, deployer fn.Deployer, remover fn.Remover, de // Deploy // ------ + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { @@ -283,6 +298,7 @@ func TestInt_Events(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -311,6 +327,12 @@ func TestInt_Events(t *testing.T, deployer fn.Deployer, remover fn.Remover, desc // Deploy // ------ + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { @@ -361,6 +383,7 @@ func TestInt_Scale(t *testing.T, deployer fn.Deployer, remover fn.Remover, descr t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -388,6 +411,12 @@ func TestInt_Scale(t *testing.T, deployer fn.Deployer, remover fn.Remover, descr }, } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { @@ -467,6 +496,7 @@ func TestInt_EnvsUpdate(t *testing.T, deployer fn.Deployer, remover fn.Remover, t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -500,6 +530,12 @@ func TestInt_EnvsUpdate(t *testing.T, deployer fn.Deployer, remover fn.Remover, // Deploy // ------ + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { diff --git a/pkg/describer/testing/integration_test_helper.go b/pkg/describer/testing/integration_test_helper.go index 8de5fd7e01..7e5db43d37 100644 --- a/pkg/describer/testing/integration_test_helper.go +++ b/pkg/describer/testing/integration_test_helper.go @@ -24,6 +24,7 @@ func TestInt_Describe(t *testing.T, describer fn.Describer, deployer fn.Deployer t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDescribers(describer), @@ -42,6 +43,12 @@ func TestInt_Describe(t *testing.T, describer fn.Describer, deployer fn.Deployer t.Fatal(err) } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { diff --git a/pkg/functions/client.go b/pkg/functions/client.go index d084d432b3..4764b750cb 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -19,7 +19,6 @@ import ( "golang.org/x/sync/errgroup" "gopkg.in/yaml.v2" - "knative.dev/func/pkg/scaffolding" "knative.dev/func/pkg/utils" ) @@ -65,6 +64,7 @@ type Client struct { repositoriesPath string // path to repositories repositoriesURI string // repo URI (overrides repositories path) verbose bool // print verbose logs + scaffolder Scaffolder // Scaffolds a function to have main builder Builder // Builds a runnable image source pusher Pusher // Pushes function image to a remote deployer Deployer // Deploys or Updates a function @@ -83,6 +83,13 @@ type Client struct { startTimeout time.Duration // default start timeout for all runs } +// Scaffolder of a function's entry (main) +type Scaffolder interface { + // Scaffold a function - create its main wrapper + // string parameter: optional path override for scaffolding destination + Scaffold(context.Context, Function, string) error +} + // Builder of function source to runnable image. type Builder interface { // Build a function project with source located at path. @@ -217,6 +224,7 @@ type MCPServer interface { func New(options ...Option) *Client { // Instantiate client with static defaults. c := &Client{ + scaffolder: &noopScaffolder{}, builder: &noopBuilder{output: os.Stdout}, pusher: &noopPusher{output: os.Stdout}, deployer: &noopDeployer{output: os.Stdout}, @@ -271,6 +279,13 @@ func WithVerbose(v bool) Option { } } +// WithScaffolder provides the implementation of a scaffolder based on builder +func WithScaffolder(s Scaffolder) Option { + return func(c *Client) { + c.scaffolder = s + } +} + // WithBuilder provides the concrete implementation of a builder. func WithBuilder(d Builder) Option { return func(c *Client) { @@ -462,14 +477,18 @@ func (c *Client) Apply(ctx context.Context, f Function) (string, Function, error // Updates a function which has already been initialized to run the latest // source code. // -// Use Apply for higher level control. Use Init, Build, Push and Deploy +// Use Apply for higher level control. Use Init, Scaffold, Build, Push and Deploy // independently for lower level control. // Returns final primary route to the Function and any errors. func (c *Client) Update(ctx context.Context, f Function) (string, Function, error) { if !f.Initialized() { return "", f, ErrNotInitialized{f.Root} } + var err error + if err = c.Scaffold(ctx, f, ""); err != nil { + return "", f, err + } if f, err = c.Build(ctx, f); err != nil { return "", f, err } @@ -477,10 +496,7 @@ func (c *Client) Update(ctx context.Context, f Function) (string, Function, erro return "", f, err } - // TODO: change this later when push doesn't return built image. - // Assign this as c.Push is going to produce the built image (for now) to - // .Deploy.Image for the deployer -- figure out where to assign .Deploy.Image - // first, might be just moved above push + // image name is generated via push to registry f.Deploy.Image = f.Build.Image if f, err = c.Deploy(ctx, f); err != nil { @@ -495,7 +511,7 @@ func (c *Client) Update(ctx context.Context, f Function) (string, Function, erro // Function. Used by Apply when the path is not yet an initialized function. // Errors if the path is already an initialized function. // -// Use Apply for higher level control. Use Init, Build, Push, Deploy +// Use Apply for higher level control. Use Init, Scaffold, Build, Push, Deploy // independently for lower level control. // // Returns the primary route to the function or error. @@ -513,6 +529,13 @@ func (c *Client) New(ctx context.Context, cfg Function) (string, Function, error return route, cfg, err } + // Scaffold the function + fmt.Fprintf(os.Stderr, "Scaffolding function\n") + err = c.Scaffold(ctx, f, "") + if err != nil { + return route, f, err + } + // Build the now-initialized function fmt.Fprintf(os.Stderr, "Building container image\n") if f, err = c.Build(ctx, f); err != nil { @@ -702,15 +725,12 @@ func (c *Client) Build(ctx context.Context, f Function, options ...BuildOption) return f, err } -// Scaffold writes a functions's scaffolding to a given path. -// It also updates the included symlink to function source 'f' to point to -// the current function's source. -func (c *Client) Scaffold(ctx context.Context, f Function, dest string) (err error) { - repo, err := NewRepository("", "") // default (embedded) repository - if err != nil { - return - } - return scaffolding.Write(dest, f.Root, f.Runtime, f.Invoke, repo.FS()) +// Scaffold scaffolds a function given scaffolder implementation at path. +// It will delete the directory contents before writing. +// 'pathOverride' is optional override of target path. Assign "" (empty string) +// for scaffolder's default to be used. +func (c *Client) Scaffold(ctx context.Context, f Function, pathOverride string) (err error) { + return c.scaffolder.Scaffold(ctx, f, pathOverride) } // printBuildActivity is a helper for ensuring the user gets feedback from @@ -1281,7 +1301,7 @@ func ensureFuncIgnore(root string) error { // Fingerprint the files at a given path. Returns a hash calculated from the // filenames and modification timestamps of the files within the given root. -// Also returns a logfile consiting of the filenames and modification times +// Also returns a logfile consisting of the filenames and modification times // which contributed to the hash. // Intended to determine if there were appreciable changes to a function's // source code, certain directories and files are ignored, such as @@ -1406,6 +1426,11 @@ func hasInitializedFunction(path string) (bool, error) { // with a minimum of external dependencies. // ----------------------------------------------------- +// Scaffolder +type noopScaffolder struct{} + +func (n *noopScaffolder) Scaffold(ctx context.Context, _ Function, _ string) error { return nil } + // Builder type noopBuilder struct{ output io.Writer } diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 7f06be1afd..14aedef473 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -125,6 +125,9 @@ func TestInt_Deploy_Defaults(t *testing.T) { if f, err = client.Init(f); err != nil { t.Fatal(err) } + if err = client.Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } @@ -658,6 +661,7 @@ func resetEnv() { func newClient(verbose bool) *fn.Client { return fn.New( fn.WithRegistry(DefaultIntTestRegistry), + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", verbose)), fn.WithPusher(oci.NewPusher(true, true, verbose)), fn.WithDeployer(knative.NewDeployer(knative.WithDeployerVerbose(verbose))), @@ -668,18 +672,15 @@ func newClient(verbose bool) *fn.Client { ) } -// copy of newClient just builder is s2i instead of buildpacks +// copy of newClient just with s2i methods instead func newClientWithS2i(verbose bool) *fn.Client { - builder := s2i.NewBuilder(s2i.WithVerbose(verbose)) - pusher := docker.NewPusher(docker.WithVerbose(verbose)) - deployer := knative.NewDeployer(knative.WithDeployerVerbose(verbose)) - return fn.New( fn.WithRegistry(DefaultIntTestRegistry), fn.WithVerbose(verbose), - fn.WithBuilder(builder), - fn.WithPusher(pusher), - fn.WithDeployer(deployer), + fn.WithScaffolder(s2i.NewScaffolder(true)), + fn.WithBuilder(s2i.NewBuilder(s2i.WithVerbose(verbose))), + fn.WithPusher(docker.NewPusher(docker.WithVerbose(verbose))), + fn.WithDeployer(knative.NewDeployer(knative.WithDeployerVerbose(verbose))), fn.WithDescribers(knative.NewDescriber(verbose), k8s.NewDescriber(verbose)), fn.WithRemovers(knative.NewRemover(verbose), k8s.NewRemover(verbose)), fn.WithListers(knative.NewLister(verbose), k8s.NewLister(verbose)), diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index 21a6725347..235e3d6801 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -692,7 +692,9 @@ func TestClient_Runner(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() ctx, cancel := context.WithCancel(context.Background()) - client := fn.New(fn.WithBuilder(oci.NewBuilder("", true)), fn.WithVerbose(true)) + client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), + fn.WithBuilder(oci.NewBuilder("", true)), fn.WithVerbose(true)) // Initialize f, err := client.Init(fn.Function{Root: root, Runtime: "go", Registry: TestRegistry}) @@ -700,6 +702,12 @@ func TestClient_Runner(t *testing.T) { t.Fatal(err) } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build if f, err = client.Build(ctx, f, fn.BuildWithPlatforms(TestPlatforms)); err != nil { t.Fatal(err) @@ -784,6 +792,7 @@ func TestClient_RunTimeout(t *testing.T) { // A client with a shorter global timeout. client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", true)), fn.WithVerbose(true), fn.WithStartTimeout(2*time.Second)) @@ -817,8 +826,13 @@ func TestClient_RunTimeout(t *testing.T) { src.Close() dst.Close() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx := t.Context() + + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } // Build if f, err = client.Build(ctx, f, fn.BuildWithPlatforms(TestPlatforms)); err != nil { @@ -1592,9 +1606,9 @@ func TestClient_Scaffold(t *testing.T) { defer rm() var out = "result" - // Assert "scaffolding" is a reserved word; not listed as aavailable + // Assert "scaffolding" is a reserved word; not listed as available // template despite being in the templates' directory. - client := fn.New() + client := fn.New(fn.WithScaffolder(oci.NewScaffolder(true))) tt, err := client.Templates().List("go") if err != nil { t.Fatal(err) @@ -2020,7 +2034,9 @@ func TestClient_RunRediness(t *testing.T) { root, cleanup := Mktemp(t) defer cleanup() - client := fn.New(fn.WithBuilder(oci.NewBuilder("", true)), fn.WithVerbose(true)) + client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), + fn.WithBuilder(oci.NewBuilder("", true)), fn.WithVerbose(true)) // Initialize f, err := client.Init(fn.Function{Root: root, Runtime: "go", Registry: TestRegistry}) @@ -2051,9 +2067,12 @@ func TestClient_RunRediness(t *testing.T) { src.Close() dst.Close() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx := t.Context() + // Scaffold + if err = client.Scaffold(ctx, f, ""); err != nil { + t.Fatal(err) + } // Build if f, err = client.Build(ctx, f, fn.BuildWithPlatforms(TestPlatforms)); err != nil { t.Fatal(err) diff --git a/pkg/functions/function.go b/pkg/functions/function.go index 4c4346f0c2..a064075a33 100644 --- a/pkg/functions/function.go +++ b/pkg/functions/function.go @@ -32,6 +32,11 @@ const ( // metadata dir (RunDataDir) BuiltHash = "built-hash" + // BuildLock indicates that Function is being currently built to prevent + // multiple builds occurring at the same time. It contains PID of the process + // that's running the build. Found in metadata dir (RunDataDir) + BuildLock = "build.lock" + // BuiltImage is a name of a file that holds name of built image in runtime // metadata dir (RunDataDir) BuiltImage = "built-image" @@ -528,7 +533,7 @@ func WithStampJournal() stampOption { // stamp is checked before certain operations, and if it has been updated, // the build can be skipped. If in doubt, just use .Write only. // -// Updates the build stamp at ./func/built (and the log +// Updates the build stamp at .func/built-hash (and the log // at .func/built.log) to reflect the current state of the filesystem. // Note that the caller should call .Write first to flush any changes to the // function in-memory to the filesystem prior to calling stamp. @@ -736,7 +741,7 @@ func (f Function) Built() bool { return true } -// BuildStamp accesses the current (last) build stamp for the function. +// BuildStamp accesses the current build stamp for the function. // Unbuilt functions return empty string. func (f Function) BuildStamp() string { path := filepath.Join(f.Root, RunDataDir, BuiltHash) diff --git a/pkg/lister/testing/integration_test_helper.go b/pkg/lister/testing/integration_test_helper.go index f9c4ca0086..3e329bd46a 100644 --- a/pkg/lister/testing/integration_test_helper.go +++ b/pkg/lister/testing/integration_test_helper.go @@ -24,6 +24,7 @@ func TestInt_List(t *testing.T, lister fn.Lister, deployer fn.Deployer, describe t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -43,6 +44,12 @@ func TestInt_List(t *testing.T, lister fn.Lister, deployer fn.Deployer, describe t.Fatal(err) } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { diff --git a/pkg/oci/builder.go b/pkg/oci/builder.go index 848a11417f..60b7d6fe20 100644 --- a/pkg/oci/builder.go +++ b/pkg/oci/builder.go @@ -112,24 +112,20 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, pp []fn.Platform) (e if err = setup(job); err != nil { // create some directories etc return } - defer cleanup(job) + defer func() { - // Always remove our own PID link when build completes + // Always remove build.lock file when build completes if job.verbose { - fmt.Fprintf(os.Stderr, "rm %v\n", job.pidLink()) + fmt.Fprintf(os.Stderr, "rm %v\n", job.lockFile()) } - _ = os.Remove(job.pidLink()) + _ = os.Remove(job.lockFile()) }() - if err = scaffold(&job); err != nil { // write out the service wrapper + if err = fetchMiddlewareLabels(&job); err != nil { return } - if err = containerize(job); err != nil { // write image to .func/builds - return - } - - if err = updateLastLink(job); err != nil { // .func/builds/last + if err = containerize(job); err != nil { // write image to .func/build return } @@ -154,15 +150,12 @@ func setup(job buildJob) (err error) { return ErrBuildInProgress{job.buildDir()} } - // Build directory - if _, err = os.Stat(job.buildDir()); !os.IsNotExist(err) { - if job.verbose { - fmt.Fprintf(os.Stderr, "rm -rf %v\n", job.buildDir()) - } - if err = os.RemoveAll(job.buildDir()); err != nil { - return - } + // Create the lock file with current PID to prevent concurrent builds + lockContent := fmt.Sprintf("%d", os.Getpid()) + if err = os.WriteFile(job.lockFile(), []byte(lockContent), 0644); err != nil { + return } + if job.verbose { fmt.Fprintf(os.Stderr, "mkdir -p %v\n", job.buildDir()) } @@ -170,25 +163,6 @@ func setup(job buildJob) (err error) { return } - // PID links directory - if _, err = os.Stat(job.pidsDir()); os.IsNotExist(err) { - if job.verbose { - fmt.Fprintf(os.Stderr, "mkdir -p %v\n", job.pidsDir()) - } - if err = os.MkdirAll(job.pidsDir(), 0774); err != nil { - return - } - } - - // Link to last build attempted (this) - target := filepath.Join("..", "by-hash", job.hash) - if job.verbose { - fmt.Fprintf(os.Stderr, "ln -s %v %v\n", target, job.pidLink()) - } - if err = os.Symlink(target, job.pidLink()); err != nil { - return err - } - // Creates the blobs directory where layer data resides // (compressed and hashed) if err := os.MkdirAll(job.blobsDir(), os.ModePerm); err != nil { @@ -209,59 +183,11 @@ func setup(job buildJob) (err error) { return } -// cleanup various filesystem artifacts of the build. -func cleanup(job buildJob) { - // cleanup orphaned build links - dd, _ := os.ReadDir(job.pidsDir()) - for _, d := range dd { - if processExists(d.Name()) { - continue - } - dir := filepath.Join(job.pidsDir(), d.Name()) - if job.verbose { - fmt.Fprintf(os.Stderr, "rm %v\n", dir) - } - _ = os.RemoveAll(dir) - } - - // remove build file directories unless they are either: - // 1. The build files from the last successful build - // 2. Are associated with a pid link (currently in progress) - dd, _ = os.ReadDir(job.buildsDir()) - for _, d := range dd { - dir := filepath.Join(job.buildsDir(), d.Name()) - if isLinkTo(job.lastLink(), dir) { - continue - } - if job.isActive() { - continue - } - if job.verbose { - fmt.Fprintf(os.Stderr, "rm %v\n", dir) - } - _ = os.RemoveAll(dir) - } -} - -// scaffold writes out the process wrapper code which will instantiate the -// Function and expose it as a service when included in the final container. -func scaffold(job *buildJob) (err error) { - // extract the embedded filesystem which holds the scaffolding for - // the given runtime +func fetchMiddlewareLabels(job *buildJob) (err error) { repo, err := fn.NewRepository("", "") if err != nil { - return + return fmt.Errorf("unable to load embedded repository: %w", err) } - err = scaffolding.Write( - job.buildDir(), // desintation for scaffolding - job.function.Root, // source to be scaffolded - job.function.Runtime, // scaffolding language to write - job.function.Invoke, repo.FS()) - - if err != nil { - return - } - // add used scaffolded middleware information middlewareVersion, err := scaffolding.MiddlewareVersion(job.function.Root, job.function.Runtime, job.function.Invoke, repo.FS()) if err != nil { @@ -317,7 +243,7 @@ func containerize(job buildJob) error { // For each platform, create a new slice of layers consisting of first // the shared layers followed by newly-generated platform-specific layers - // for each platform. Bundle these into a new image manifest for inclusion + // for each platform. Bundle these into a new image manifest for inclusion // in the final container. for _, p := range job.platforms { @@ -932,42 +858,37 @@ func newBuildJob(ctx context.Context, f fn.Function, pp []fn.Platform, verbose b // some convenience accessors -func (j buildJob) lastLink() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "last") -} -func (j buildJob) pidsDir() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-pid") -} -func (j buildJob) pidLink() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-pid", strconv.Itoa(os.Getpid())) +func (j buildJob) buildDir() string { + return filepath.Join(j.function.Root, fn.RunDataDir, "build") } -func (j buildJob) buildsDir() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-hash") + +func (j buildJob) lockFile() string { + return filepath.Join(j.function.Root, fn.RunDataDir, "build.lock") } -func (j buildJob) buildDir() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-hash", j.hash) + +func (j buildJob) lockFilePid(content []byte) string { + return strings.TrimSpace(string(content)) } + func (j buildJob) ociDir() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-hash", j.hash, "oci") + return filepath.Join(j.function.Root, fn.RunDataDir, "build", "oci") } func (j buildJob) blobsDir() string { - return filepath.Join(j.function.Root, fn.RunDataDir, "builds", "by-hash", j.hash, "oci", "blobs", "sha256") + return filepath.Join(j.function.Root, fn.RunDataDir, "build", "oci", "blobs", "sha256") } func (j buildJob) cacheDir() string { return filepath.Join(j.function.Root, fn.RunDataDir, "blob-cache") } -// isActive returns false if an active build for this Function is detected. +// isActive returns true if an active build for this Function is detected. func (j buildJob) isActive() bool { - dd, _ := os.ReadDir(j.pidsDir()) - for _, d := range dd { - // for each link in PIDs dir - // the build is active if a process exists of the same name - // AND it is a link to this job's build directory. - link := filepath.Join(j.pidsDir(), d.Name()) - if processExists(d.Name()) && isLinkTo(link, j.buildDir()) { - return true - } + d, err := os.ReadFile(j.lockFile()) + if err != nil { + return false + } + // build is active only if the lock file AND its process exists + if processExists(j.lockFilePid(d)) { + return true } return false } @@ -982,14 +903,14 @@ func (j buildJob) isActive() bool { // build directory. If it is not a subpath, the full path is returned // unchanged. func rel(base, path string) string { - if strings.HasPrefix(path, base) { - return "." + strings.TrimPrefix(path, base) + s, cut := strings.CutPrefix(path, base) + if cut { + return "." + s } return path } -// processExists returns true if the process with the given PID -// exists. +// processExists returns true if the process with the given PID exists. func processExists(pid string) bool { p, err := strconv.Atoi(pid) if err != nil { @@ -1006,38 +927,6 @@ func processExists(pid string) bool { return err == nil } -// isLinkTo returns true if link is a link to target. -func isLinkTo(link, target string) bool { - var err error - if link, err = filepath.EvalSymlinks(link); err != nil { - return false - } - if link, err = filepath.Abs(link); err != nil { - return false - } - - if target, err = filepath.EvalSymlinks(target); err != nil { - return false - } - if target, err = filepath.Abs(target); err != nil { - return false - } - - return link == target -} - -func updateLastLink(job buildJob) error { - if job.verbose { - fmt.Fprintf(os.Stderr, "ln -s %v %v\n", job.buildDir(), job.lastLink()) - } - _ = os.RemoveAll(job.lastLink()) - rp, err := filepath.Rel(filepath.Dir(job.lastLink()), job.buildDir()) - if err != nil { - return err - } - return os.Symlink(rp, job.lastLink()) -} - // toPlatforms converts func's implementation-agnostic Platform struct // into to the OCI builder's implementation-specific go-containerregistry v1 // palatform. diff --git a/pkg/oci/builder_test.go b/pkg/oci/builder_test.go index af4cfdf9c9..c41df5c4f3 100644 --- a/pkg/oci/builder_test.go +++ b/pkg/oci/builder_test.go @@ -73,15 +73,21 @@ func TestBuilder_BuildGo(t *testing.T) { t.Fatal(err) } + scaffolder := NewScaffolder(true) builder := NewBuilder("", true) - if err := builder.Build(context.Background(), f, TestPlatforms); err != nil { + ctx := t.Context() + if err := scaffolder.Scaffold(ctx, f, ""); err != nil { t.Fatal(err) } - last := filepath.Join(f.Root, fn.RunDataDir, "builds", "last", "oci") + if err := builder.Build(ctx, f, TestPlatforms); err != nil { + t.Fatal(err) + } - validateOCIStructure(last, t) // validate OCI compliant + oci := filepath.Join(f.Root, fn.RunDataDir, "build", "oci") + + validateOCIStructure(oci, t) // validate OCI compliant } // TestBuilder_BuildPython ensures that, when given a Python Function, an @@ -102,16 +108,20 @@ func TestBuilder_BuildPython(t *testing.T) { if err != nil { t.Fatal(err) } - + ctx := t.Context() + scaffolder := NewScaffolder(true) builder := NewBuilder("", true) - if err := builder.Build(context.Background(), f, TestPlatforms); err != nil { + if err := scaffolder.Scaffold(ctx, f, ""); err != nil { + t.Fatal(err) + } + if err := builder.Build(ctx, f, TestPlatforms); err != nil { t.Fatal(err) } - last := filepath.Join(f.Root, fn.RunDataDir, "builds", "last", "oci") + oci := filepath.Join(f.Root, fn.RunDataDir, "build", "oci") - validateOCIStructure(last, t) // validate OCI compliant + validateOCIStructure(oci, t) // validate OCI compliant } // TestBuilder_Files ensures that static files are added to the container @@ -153,6 +163,11 @@ func TestBuilder_Files(t *testing.T) { } } + // Scaffold first to copy certs and scaffolding files + if err := NewScaffolder(true).Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } + if err := NewBuilder("", true).Build(context.Background(), f, TestPlatforms); err != nil { t.Fatal(err) } @@ -171,12 +186,16 @@ func TestBuilder_Files(t *testing.T) { {Path: "/func/handle_test.go"}, } - last := filepath.Join(f.Root, fn.RunDataDir, "builds", "last", "oci") + oci := filepath.Join(f.Root, fn.RunDataDir, "build", "oci") - validateOCIFiles(last, expected, t) + validateOCIFiles(oci, expected, t) } -// TestBuilder_Concurrency +// TestBuilder_Concurrency tests the full build flow including lock file behavior: +// 1. Lock file exists during build with correct PID +// 2. Concurrent build is blocked with ErrBuildInProgress +// 3. Lock file is cleaned up after build completes +// 4. Subsequent builds succeed after lock is released func TestBuilder_Concurrency(t *testing.T) { root, done := Mktemp(t) defer done() @@ -189,6 +208,13 @@ func TestBuilder_Concurrency(t *testing.T) { t.Fatal(err) } + // Scaffold first to copy certs and scaffolding files + if err := NewScaffolder(true).Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } + + lockFile := filepath.Join(f.Root, fn.RunDataDir, "build.lock") + // Concurrency // // The first builder is setup to use a mock implementation of the @@ -235,7 +261,22 @@ func TestBuilder_Concurrency(t *testing.T) { // Wait until build 1 indicates it is paused <-pausedCh - // Build B + // Verify lock file exists during build + if _, err := os.Stat(lockFile); os.IsNotExist(err) { + t.Error("lock file should exist during build") + } + + // Verify lock file contains current process PID + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Errorf("failed to read lock file: %v", err) + } + expectedPID := strconv.Itoa(os.Getpid()) + if strings.TrimSpace(string(lockContent)) != expectedPID { + t.Errorf("lock file contains %q, expected %q", string(lockContent), expectedPID) + } + + // Build B - should fail with ErrBuildInProgress builder2 := NewBuilder("builder2", true) testImplB := NewTestLanguageBuilder() testImplB.WritePlatformFn = func(job buildJob, p v1.Platform) ([]imageLayer, error) { @@ -246,13 +287,29 @@ func TestBuilder_Concurrency(t *testing.T) { defer wg.Done() err = builder2.Build(context.Background(), f, TestPlatforms) if !errors.As(err, &ErrBuildInProgress{}) { - t.Errorf("test build error: %v", err) + t.Errorf("expected ErrBuildInProgress, got: %v", err) } }() // Release the blocking Build A and wait until complete. continueCh <- true wg.Wait() + + // Verify lock file is cleaned up after build completes + if _, err := os.Stat(lockFile); !os.IsNotExist(err) { + t.Error("lock file should be removed after build completes") + } + + // Build C - verify subsequent build succeeds after lock released + builder3 := NewBuilder("builder3", true) + if err := builder3.Build(context.Background(), f, TestPlatforms); err != nil { + t.Errorf("subsequent build should succeed after lock released: %v", err) + } + + // Verify lock file cleaned up after Build C + if _, err := os.Stat(lockFile); !os.IsNotExist(err) { + t.Error("lock file should be removed after subsequent build") + } } func isFirstBuild(cfg buildJob, current v1.Platform) bool { @@ -416,6 +473,11 @@ func TestBuilder_StaticEnvs(t *testing.T) { t.Fatal(err) } + // Scaffold first to copy certs and scaffolding files + if err := NewScaffolder(true).Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } + if err := NewBuilder("", true).Build(context.Background(), f, TestPlatforms); err != nil { t.Fatal(err) } @@ -425,7 +487,7 @@ func TestBuilder_StaticEnvs(t *testing.T) { // variables on each of the constituent containers. // --- // Get the images list (manifest descripors) from the index - ociPath := filepath.Join(f.Root, fn.RunDataDir, "builds", "last", "oci") + ociPath := filepath.Join(f.Root, fn.RunDataDir, "build", "oci") data, err := os.ReadFile(filepath.Join(ociPath, "index.json")) if err != nil { t.Fatal(err) @@ -538,6 +600,63 @@ func (l *TestLanguageBuilder) Configure(job buildJob, p v1.Platform, c v1.Config return l.ConfigureFn(job, p, c) } +// TestBuilder_LockLifecycle tests the build lock mechanism with 3 scenarios: +// 1. Clean build succeeds +// 2. Active lock (current PID) blocks build with ErrBuildInProgress +// 3. Stale lock (dead PID) allows build to proceed +func TestBuilder_LockLifecycle(t *testing.T) { + root, done := Mktemp(t) + defer done() + + f, err := fn.New().Init(fn.Function{Root: root, Runtime: "go"}) + if err != nil { + t.Fatal(err) + } + + if err := NewScaffolder(true).Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } + + lockFile := filepath.Join(f.Root, fn.RunDataDir, "build.lock") + ctx := context.Background() + + // --- Scenario 1: Clean build succeeds --- + if err := NewBuilder("", true).Build(ctx, f, TestPlatforms); err != nil { + t.Fatalf("clean build should succeed: %v", err) + } + + // --- Scenario 2: Active lock (current PID) blocks build --- + // Write current process PID to simulate an active build + // + // In case we implement some kind of process self-knowledge (checking if + // I -as a process- am running this build, therefore am the one locking the + // build, perhaps we can change this to os.Getppid() instead. + if err := os.MkdirAll(filepath.Dir(lockFile), 0755); err != nil { + t.Fatal(err) + } + + if err := os.WriteFile(lockFile, []byte(strconv.Itoa(os.Getpid())), 0644); err != nil { + t.Fatal(err) + } + + err = NewBuilder("", true).Build(ctx, f, TestPlatforms) + if !errors.As(err, &ErrBuildInProgress{}) { + t.Fatalf("expected ErrBuildInProgress with active lock, got: %v", err) + } + + os.Remove(lockFile) // cleanup for next scenario + + // --- Scenario 3: Stale lock (dead PID) allows build --- + // Write a non-existent PID to simulate a stale lock from crashed build + if err := os.WriteFile(lockFile, []byte("99999999"), 0644); err != nil { + t.Fatal(err) + } + + if err := NewBuilder("", true).Build(ctx, f, TestPlatforms); err != nil { + t.Fatalf("build with stale lock should succeed: %v", err) + } +} + // Test_validatedLinkTarget ensures that the function disallows // links which are absolute or refer to targets outside the given root, in // addition to the basic job of returning the value of reading the link. diff --git a/pkg/oci/go_builder.go b/pkg/oci/go_builder.go index c13358dd74..3fe8cb0943 100644 --- a/pkg/oci/go_builder.go +++ b/pkg/oci/go_builder.go @@ -124,7 +124,7 @@ func goBuildCmd(p v1.Platform, cfg buildJob) (gobin string, args []string, outpa gobin = "go" } - // Build as ./func/builds/$PID/result/f.$OS.$Architecture + // Build as ./func/build/result/f.$OS.$Architecture name := fmt.Sprintf("f.%v.%v", p.OS, p.Architecture) if p.Variant != "" { name = name + "." + p.Variant diff --git a/pkg/oci/pusher.go b/pkg/oci/pusher.go index 07b97d4791..8bfe81a54b 100644 --- a/pkg/oci/pusher.go +++ b/pkg/oci/pusher.go @@ -94,7 +94,7 @@ func (p *Pusher) Push(ctx context.Context, f fn.Function) (digest string, err er go p.handleUpdates(ctx) defer func() { p.done <- true }() - buildDir, err := getLastBuildDir(f) + buildDir, err := getBuildDir(f) if err != nil { return } @@ -157,11 +157,11 @@ func (p *Pusher) handleUpdates(ctx context.Context) { } } -// The last build directory is symlinked upon successful build. -func getLastBuildDir(f fn.Function) (string, error) { - dir := filepath.Join(f.Root, fn.RunDataDir, "builds", "last") +// getBuildDir returns the build directory +func getBuildDir(f fn.Function) (string, error) { + dir := filepath.Join(f.Root, fn.RunDataDir, "build") if _, err := os.Stat(dir); os.IsNotExist(err) { - return dir, fmt.Errorf("last build directory not found '%v'. Has it been built?", dir) + return dir, fmt.Errorf("build directory not found '%v'. Has it been built?", dir) } return dir, nil } diff --git a/pkg/oci/pusher_test.go b/pkg/oci/pusher_test.go index d6fd3b6701..21d04d4fd6 100644 --- a/pkg/oci/pusher_test.go +++ b/pkg/oci/pusher_test.go @@ -61,6 +61,7 @@ func TestPusher_Push(t *testing.T) { // Create and push a function client := fn.New( + fn.WithScaffolder(NewScaffolder(false)), fn.WithBuilder(NewBuilder("", false)), fn.WithPusher(NewPusher(insecure, anon, verbose))) @@ -70,6 +71,10 @@ func TestPusher_Push(t *testing.T) { t.Fatal(err) } + if err = client.Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } + if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } @@ -135,6 +140,7 @@ func TestPusher_BasicAuth(t *testing.T) { // Client // initialized with an OCI builder and pusher. client := fn.New( + fn.WithScaffolder(NewScaffolder(verbose)), fn.WithBuilder(NewBuilder("", verbose)), fn.WithPusher(pusher), ) @@ -150,6 +156,9 @@ func TestPusher_BasicAuth(t *testing.T) { if f, err = client.Init(f); err != nil { t.Fatal(err) } + if err = client.Scaffold(context.Background(), f, ""); err != nil { + t.Fatal(err) + } if f, err = client.Build(context.Background(), f); err != nil { t.Fatal(err) } diff --git a/pkg/oci/python_builder.go b/pkg/oci/python_builder.go index 4272590928..af13906869 100644 --- a/pkg/oci/python_builder.go +++ b/pkg/oci/python_builder.go @@ -41,8 +41,8 @@ func (b pythonBuilder) Base(customBase string) string { // ConfigFile that will be used when building the template. func (b pythonBuilder) Configure(job buildJob, _ v1.Platform, cf v1.ConfigFile) (v1.ConfigFile, error) { var ( - svcRelPath, _ = filepath.Rel(job.function.Root, job.buildDir()) // eg .func/builds/by-hash/$HASH - svcPath = filepath.Join("/func", svcRelPath) // eg /func/.func/builds/by-hash/$HASH + svcRelPath, _ = filepath.Rel(job.function.Root, job.buildDir()) // .func/build + svcPath = filepath.Join("/func", svcRelPath) // /func/.func/build pythonPathEnv = fmt.Sprintf("PYTHONPATH=%v/lib", svcPath) mainPath = fmt.Sprintf("%v/service/main.py", svcPath) listenAddrEnv = "LISTEN_ADDRESS=[::]:8080" @@ -128,7 +128,7 @@ func (b pythonBuilder) WriteShared(job buildJob) (layers []imageLayer, err error func newPythonLibTarball(job buildJob, root, target string) error { // Create a tarball of the "build directory" // when extracted, it's root will be /func - // all files within should have path prefix .func/builds/by-hash/$hash + // all files within should have path prefix .func/build targetFile, err := os.Create(target) // final .tar.gz if err != nil { diff --git a/pkg/oci/scaffolder.go b/pkg/oci/scaffolder.go new file mode 100644 index 0000000000..429db0dd69 --- /dev/null +++ b/pkg/oci/scaffolder.go @@ -0,0 +1,51 @@ +package oci + +import ( + "context" + "fmt" + "os" + "path/filepath" + + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/scaffolding" +) + +const ( + defaultPath = ".func/build" +) + +type Scaffolder struct { + verbose bool +} + +func NewScaffolder(verbose bool) *Scaffolder { + return &Scaffolder{verbose: verbose} +} + +// Scaffold the function so that it can be built via oci builder. +// 'path' is an optional override. Assign "" (empty string) most of the time +func (s Scaffolder) Scaffold(ctx context.Context, f fn.Function, path string) error { + if f.Runtime != "go" && f.Runtime != "python" { + if s.verbose { + fmt.Println("Scaffolding skipped. Currently available for runtimes go & python") + } + return nil + } + + appRoot := path + if appRoot == "" { + appRoot = filepath.Join(f.Root, defaultPath) + } + if s.verbose { + fmt.Printf("Writing host scaffolding at path '%v'\n", appRoot) + } + embeddedRepo, err := fn.NewRepository("", "") + if err != nil { + return fmt.Errorf("unable to load embedded scaffolding: %w", err) + } + _ = os.RemoveAll(appRoot) + if err := scaffolding.Write(appRoot, f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()); err != nil { + return err + } + return nil +} diff --git a/pkg/pipelines/tekton/tasks.go b/pkg/pipelines/tekton/tasks.go index 9da22c8b55..011ab0a3b8 100644 --- a/pkg/pipelines/tekton/tasks.go +++ b/pkg/pipelines/tekton/tasks.go @@ -445,6 +445,9 @@ spec: - name: path description: Path to the function project default: "" + - name: builder + description: Builder to be used (pack or s2i) + default: "" results: - name: middlewareVersion workspaces: @@ -453,7 +456,7 @@ spec: steps: - name: func-scaffold image: %s - command: ["scaffold", "$(params.path)"] + command: ["scaffold", "$(params.path)", "$(params.builder)"] `, ScaffoldImage) } diff --git a/pkg/pipelines/tekton/templates.go b/pkg/pipelines/tekton/templates.go index cd2499f8b3..7e85d7c09c 100644 --- a/pkg/pipelines/tekton/templates.go +++ b/pkg/pipelines/tekton/templates.go @@ -413,6 +413,11 @@ func createAndApplyPipelineRunTemplate(f fn.Function, namespace string, labels m } } + // add BP_GO_WORKDIR for go-build buildpack + if f.Runtime == "go" { + buildEnvs = append(buildEnvs, "BP_GO_WORKDIR=.func/build") + } + s2iImageScriptsUrl := defaultS2iImageScriptsUrl if f.Runtime == "quarkus" { s2iImageScriptsUrl = quarkusS2iImageScriptsUrl diff --git a/pkg/pipelines/tekton/templates_pack.go b/pkg/pipelines/tekton/templates_pack.go index e9ab0e3953..60f67d0987 100644 --- a/pkg/pipelines/tekton/templates_pack.go +++ b/pkg/pipelines/tekton/templates_pack.go @@ -46,6 +46,8 @@ spec: params: - name: path value: $(workspaces.source.path)/$(params.contextDir) + - name: builder + value: pack workspaces: - name: source workspace: source-workspace diff --git a/pkg/pipelines/tekton/templates_s2i.go b/pkg/pipelines/tekton/templates_s2i.go index e938e9677b..f10e945488 100644 --- a/pkg/pipelines/tekton/templates_s2i.go +++ b/pkg/pipelines/tekton/templates_s2i.go @@ -58,6 +58,8 @@ spec: params: - name: path value: $(workspaces.source.path)/$(params.contextDir) + - name: builder + value: s2i workspaces: - name: source workspace: source-workspace diff --git a/pkg/remover/testing/integration_test_helper.go b/pkg/remover/testing/integration_test_helper.go index adc9d320d7..fbe9de2667 100644 --- a/pkg/remover/testing/integration_test_helper.go +++ b/pkg/remover/testing/integration_test_helper.go @@ -24,6 +24,7 @@ func TestInt_Remove(t *testing.T, remover fn.Remover, deployer fn.Deployer, desc t.Cleanup(cancel) client := fn.New( + fn.WithScaffolder(oci.NewScaffolder(true)), fn.WithBuilder(oci.NewBuilder("", false)), fn.WithPusher(oci.NewPusher(true, true, true)), fn.WithDeployer(deployer), @@ -43,6 +44,12 @@ func TestInt_Remove(t *testing.T, remover fn.Remover, deployer fn.Deployer, desc t.Fatal(err) } + // Scaffold + err = client.Scaffold(ctx, f, "") + if err != nil { + t.Fatal(err) + } + // Build f, err = client.Build(ctx, f) if err != nil { diff --git a/pkg/s2i/assemblers.go b/pkg/s2i/assemblers.go index 0461e0998b..73bbba37f8 100644 --- a/pkg/s2i/assemblers.go +++ b/pkg/s2i/assemblers.go @@ -41,7 +41,7 @@ func assembler(f fn.Function) (string, error) { // GoAssembler // // Adapted from /usr/libexec/s2i/assemble within the UBI-8 go-toolchain -// such that the "go build" command builds subdirectory .s2i/builds/last +// such that the "go build" command builds subdirectory .s2i/build // (where main resides) rather than the root. const GoAssembler = ` #!/bin/bash @@ -76,7 +76,7 @@ if [[ $(go list -f {{.Incomplete}}) == "true" ]]; then /$STI_SCRIPTS_PATH/usage exit 1 else - pushd .s2i/builds/last + pushd .s2i/build go mod tidy go build -o /opt/app-root/gobinary popd @@ -87,9 +87,9 @@ fi // PythonAssembler // // Adapted from /usr/libexec/s2i/assemble within the UBI-8 python-toolchain -// such that the the script executes from subdirectory .s2i/builds/last +// such that the the script executes from subdirectory .s2i/build // (where main resides) rather than the root, and indicates the main is -// likewise in .s2i/builds/last/service/main.py via Procfile. See the comment +// likewise in .s2i/build/service/main.py via Procfile. See the comment // inline on line 50 of the script for where the directory change instruction // was added. const PythonAssembler = ` @@ -151,12 +151,12 @@ echo "---> (Functions) Writing app.sh ..." cat << 'EOF' > app.sh #!/bin/bash set -e -exec python .s2i/builds/last/service/main.py +exec python .s2i/build/service/main.py EOF chmod +x app.sh -echo "---> (Functions) Changing directory to .s2i/builds/last ..." -cd .s2i/builds/last +echo "---> (Functions) Changing directory to .s2i/build ..." +cd .s2i/build # END MODIFICATION FOR FUNCTIONS # ------------------------------ diff --git a/pkg/s2i/builder.go b/pkg/s2i/builder.go index 9aae94b13f..4e4e0dd416 100644 --- a/pkg/s2i/builder.go +++ b/pkg/s2i/builder.go @@ -21,7 +21,6 @@ import ( "knative.dev/func/pkg/builders" "knative.dev/func/pkg/docker" fn "knative.dev/func/pkg/functions" - "knative.dev/func/pkg/scaffolding" ) // DefaultName when no WithName option is provided to NewBuilder @@ -50,16 +49,16 @@ type Builder struct { cli s2idocker.Client } -type Option func(*Builder) +type BuilderOption func(*Builder) -func WithName(n string) Option { +func WithName(n string) BuilderOption { return func(b *Builder) { b.name = n } } // WithVerbose toggles verbose logging. -func WithVerbose(v bool) Option { +func WithVerbose(v bool) BuilderOption { return func(b *Builder) { b.verbose = v } @@ -68,20 +67,20 @@ func WithVerbose(v bool) Option { // WithImpl sets an optional S2I Builder implementation override to use in // place of what will be generated by the S2I build "strategy" system based // on the config. Used for mocking the implementation during tests. -func WithImpl(s build.Builder) Option { +func WithImpl(s build.Builder) BuilderOption { return func(b *Builder) { b.impl = s } } -func WithDockerClient(cli s2idocker.Client) Option { +func WithDockerClient(cli s2idocker.Client) BuilderOption { return func(b *Builder) { b.cli = cli } } // NewBuilder creates a new instance of a Builder with static defaults. -func NewBuilder(options ...Option) *Builder { +func NewBuilder(options ...BuilderOption) *Builder { b := &Builder{name: DefaultName} for _, o := range options { o(b) @@ -158,20 +157,22 @@ func (b *Builder) Build(ctx context.Context, f fn.Function, platforms []fn.Platf PreviousImagePullPolicy: api.DefaultPreviousImagePullPolicy, RuntimeImagePullPolicy: api.DefaultRuntimeImagePullPolicy, DockerConfig: s2idocker.GetDefaultDockerConfig(), + // scaffolding additions + KeepSymlinks: true, // Don't infinite loop on the symlink to root. + // We want to force that the system use the (copy via filesystem) + // method rather than a "git clone" method because (other than being + // faster) appears to have a bug where the assemble script is ignored. + // Maybe this issue is related: + // https://github.com/openshift/source-to-image/issues/1141 + ForceCopy: true, + // Excludes + // Do not include .git, .env, .func or any language-specific cache directories + // (node_modules, etc) in the tar file sent to the builder, as this both + // bloats the build process and can cause unexpected errors in the resultant + // function. + ExcludeRegExp: "(^|/)\\.git|\\.env|\\.func|node_modules(/|$)", } - // Scaffold - if cfg, err = scaffold(cfg, f); err != nil { - return - } - - // Excludes - // Do not include .git, .env, .func or any language-specific cache directories - // (node_modules, etc) in the tar file sent to the builder, as this both - // bloats the build process and can cause unexpected errors in the resultant - // function. - cfg.ExcludeRegExp = "(^|/)\\.git|\\.env|\\.func|node_modules(/|$)" - // Environment variables // Build Envs have local env var references interpolated then added to the // config as an S2I EnvironmentList struct @@ -233,68 +234,3 @@ func BuilderImage(f fn.Function, builderName string) (string, error) { // delegate as the logic is shared amongst builders return builders.Image(f, builderName, DefaultBuilderImages) } - -// scaffold the project -// Returns a config with settings suitable for building runtimes which -// support scaffolding. -func scaffold(cfg *api.Config, f fn.Function) (*api.Config, error) { - // Scaffolding is currently only supported by the Go and Python runtimes - if f.Runtime != "go" && f.Runtime != "python" { - return cfg, nil - } - - contextDir := filepath.Join(".s2i", "builds", "last") - appRoot := filepath.Join(f.Root, contextDir) - _ = os.RemoveAll(appRoot) - - // The embedded repository contains the scaffolding code itself which glues - // together the middleware and a function via main - embeddedRepo, err := fn.NewRepository("", "") // default is the embedded fs - if err != nil { - return cfg, fmt.Errorf("unable to load the embedded scaffolding. %w", err) - } - - // Write scaffolding to .s2i/builds/last - err = scaffolding.Write(appRoot, f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()) - if err != nil { - return cfg, fmt.Errorf("unable to build due to a scaffold error. %w", err) - } - - // Write out an S2I assembler script if the runtime needs to override the - // one provided in the S2I image. - assemble, err := assembler(f) - if err != nil { - return cfg, err - } - if assemble != "" { - if err := os.MkdirAll(filepath.Join(f.Root, ".s2i", "bin"), 0755); err != nil { - return nil, fmt.Errorf("unable to create .s2i bin dir. %w", err) - } - if err := os.WriteFile(filepath.Join(f.Root, ".s2i", "bin", "assemble"), []byte(assemble), 0700); err != nil { - return nil, fmt.Errorf("unable to write go assembler. %w", err) - } - } - - cfg.KeepSymlinks = true // Don't infinite loop on the symlink to root. - - // We want to force that the system use the (copy via filesystem) - // method rather than a "git clone" method because (other than being - // faster) appears to have a bug where the assemble script is ignored. - // Maybe this issue is related: - // https://github.com/openshift/source-to-image/issues/1141 - cfg.ForceCopy = true - - // add used scaffolded middleware information - middlewareVersion, err := scaffolding.MiddlewareVersion(f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()) - if err != nil { - return cfg, fmt.Errorf("unable to get middleware version: %w", err) - } - - if cfg.Labels == nil { - cfg.Labels = make(map[string]string) - } - - cfg.Labels[fn.MiddlewareVersionLabelKey] = middlewareVersion - - return cfg, nil -} diff --git a/pkg/s2i/scaffolder.go b/pkg/s2i/scaffolder.go new file mode 100644 index 0000000000..41dd72c277 --- /dev/null +++ b/pkg/s2i/scaffolder.go @@ -0,0 +1,65 @@ +package s2i + +import ( + "context" + "fmt" + "os" + "path/filepath" + + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/scaffolding" +) + +const ( + defaultPath = ".s2i/build" +) + +type Scaffolder struct { + verbose bool +} + +func NewScaffolder(verbose bool) *Scaffolder { + return &Scaffolder{verbose: verbose} +} + +// Scaffold the function so that it can be built via s2i builder. +// 'path' is an optional override. Assign "" (empty string) most of the time +func (s Scaffolder) Scaffold(ctx context.Context, f fn.Function, path string) error { + if f.Runtime != "go" && f.Runtime != "python" { + if s.verbose { + fmt.Println("Scaffolding skipped. Currently available for runtimes go & python") + } + return nil + } + appRoot := path + if appRoot == "" { + appRoot = filepath.Join(f.Root, defaultPath) + } + if s.verbose { + fmt.Printf("Writing s2i scaffolding at path '%v'\n", appRoot) + } + embeddedRepo, err := fn.NewRepository("", "") + if err != nil { + return fmt.Errorf("unable to load embedded scaffolding: %w", err) + } + _ = os.RemoveAll(appRoot) + if err := scaffolding.Write(appRoot, f.Root, f.Runtime, f.Invoke, embeddedRepo.FS()); err != nil { + return err + } + + // Write out an S2I assembler script if the runtime needs to override the + // one provided in the S2I image. + assemble, err := assembler(f) + if err != nil { + return err + } + if assemble != "" { + if err := os.MkdirAll(filepath.Join(f.Root, ".s2i", "bin"), 0755); err != nil { + return fmt.Errorf("unable to create .s2i bin dir. %w", err) + } + if err := os.WriteFile(filepath.Join(f.Root, ".s2i", "bin", "assemble"), []byte(assemble), 0700); err != nil { + return fmt.Errorf("unable to write assembler script. %w", err) + } + } + return nil +}