From 32a45cde3bd2ef806032d237b38b4d022774a2c2 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 26 May 2023 15:22:01 -0400 Subject: [PATCH] 20540: Remove arvadosclient usage in copier and git_tree. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/crunchrun/copier.go | 3 +- lib/crunchrun/copier_test.go | 4 -- lib/crunchrun/crunchrun.go | 3 +- lib/crunchrun/crunchrun_test.go | 71 ++++++++++++++++++++++++++++++++- lib/crunchrun/git_mount.go | 15 +++---- lib/crunchrun/git_mount_test.go | 35 +++++----------- 6 files changed, 87 insertions(+), 44 deletions(-) diff --git a/lib/crunchrun/copier.go b/lib/crunchrun/copier.go index 72c714dfa4..3b37227fa5 100644 --- a/lib/crunchrun/copier.go +++ b/lib/crunchrun/copier.go @@ -51,7 +51,6 @@ type filetodo struct { // manifest, err := (&copier{...}).Copy() type copier struct { client *arvados.Client - arvClient IArvadosClient keepClient IKeepClient hostOutputDir string ctrOutputDir string @@ -356,7 +355,7 @@ func (cp *copier) getManifest(pdh string) (*manifest.Manifest, error) { return mft, nil } var coll arvados.Collection - err := cp.arvClient.Get("collections", pdh, nil, &coll) + err := cp.client.RequestAndDecode(&coll, "GET", "arvados/v1/collections/"+pdh, nil, nil) if err != nil { return nil, fmt.Errorf("error retrieving collection record for %q: %s", pdh, err) } diff --git a/lib/crunchrun/copier_test.go b/lib/crunchrun/copier_test.go index 5e92490163..c8936d1a9f 100644 --- a/lib/crunchrun/copier_test.go +++ b/lib/crunchrun/copier_test.go @@ -12,7 +12,6 @@ import ( "syscall" "git.arvados.org/arvados.git/sdk/go/arvados" - "git.arvados.org/arvados.git/sdk/go/arvadosclient" "git.arvados.org/arvados.git/sdk/go/arvadostest" "github.com/sirupsen/logrus" check "gopkg.in/check.v1" @@ -27,12 +26,9 @@ type copierSuite struct { func (s *copierSuite) SetUpTest(c *check.C) { tmpdir := c.MkDir() - api, err := arvadosclient.MakeArvadosClient() - c.Assert(err, check.IsNil) s.log = bytes.Buffer{} s.cp = copier{ client: arvados.NewClientFromEnv(), - arvClient: api, hostOutputDir: tmpdir, ctrOutputDir: "/ctr/outdir", mounts: map[string]arvados.Mount{ diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go index 4a514f3d89..cab09d11c0 100644 --- a/lib/crunchrun/crunchrun.go +++ b/lib/crunchrun/crunchrun.go @@ -638,7 +638,7 @@ func (runner *ContainerRunner) SetupMounts() (map[string]bindmount, error) { if err != nil { return nil, fmt.Errorf("creating temp dir: %v", err) } - err = gitMount(mnt).extractTree(runner.ContainerArvClient, tmpdir, token) + err = gitMount(mnt).extractTree(runner.containerClient, tmpdir, token) if err != nil { return nil, err } @@ -1345,7 +1345,6 @@ func (runner *ContainerRunner) CaptureOutput(bindmounts map[string]bindmount) er txt, err := (&copier{ client: runner.containerClient, - arvClient: runner.ContainerArvClient, keepClient: runner.ContainerKeepClient, hostOutputDir: runner.HostOutputDir, ctrOutputDir: runner.Container.OutputPath, diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go index 9c4fe20bc1..aa20104f33 100644 --- a/lib/crunchrun/crunchrun_test.go +++ b/lib/crunchrun/crunchrun_test.go @@ -17,6 +17,8 @@ import ( "math/rand" "net/http" "net/http/httptest" + "net/http/httputil" + "net/url" "os" "os/exec" "path" @@ -38,6 +40,8 @@ import ( "git.arvados.org/arvados.git/sdk/go/manifest" . "gopkg.in/check.v1" + git_client "gopkg.in/src-d/go-git.v4/plumbing/transport/client" + git_http "gopkg.in/src-d/go-git.v4/plumbing/transport/http" ) // Gocheck boilerplate @@ -416,6 +420,67 @@ func (client *KeepTestClient) ManifestFileReader(m manifest.Manifest, filename s return nil, nil } +type apiStubServer struct { + server *httptest.Server + proxy *httputil.ReverseProxy + intercept func(http.ResponseWriter, *http.Request) bool + + container arvados.Container + logs map[string]string +} + +func apiStub() (*arvados.Client, *apiStubServer) { + client := arvados.NewClientFromEnv() + apistub := &apiStubServer{} + apistub.server = httptest.NewTLSServer(apistub) + apistub.proxy = httputil.NewSingleHostReverseProxy(&url.URL{Scheme: "https", Host: client.APIHost}) + if client.Insecure { + apistub.proxy.Transport = arvados.InsecureHTTPClient.Transport + } + client.APIHost = apistub.server.Listener.Addr().String() + return client, apistub +} + +func (apistub *apiStubServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if apistub.intercept != nil && apistub.intercept(w, r) { + return + } + if r.Method == "POST" && r.URL.Path == "/arvados/v1/logs" { + var body struct { + Log struct { + EventType string `json:"event_type"` + Properties struct { + Text string + } + } + } + json.NewDecoder(r.Body).Decode(&body) + apistub.logs[body.Log.EventType] += body.Log.Properties.Text + return + } + if r.Method == "GET" && r.URL.Path == "/arvados/v1/collections/"+hwPDH { + json.NewEncoder(w).Encode(arvados.Collection{ManifestText: hwManifest}) + return + } + if r.Method == "GET" && r.URL.Path == "/arvados/v1/collections/"+otherPDH { + json.NewEncoder(w).Encode(arvados.Collection{ManifestText: otherManifest}) + return + } + if r.Method == "GET" && r.URL.Path == "/arvados/v1/collections/"+normalizedWithSubdirsPDH { + json.NewEncoder(w).Encode(arvados.Collection{ManifestText: normalizedManifestWithSubdirs}) + return + } + if r.Method == "GET" && r.URL.Path == "/arvados/v1/collections/"+denormalizedWithSubdirsPDH { + json.NewEncoder(w).Encode(arvados.Collection{ManifestText: denormalizedManifestWithSubdirs}) + return + } + if r.Method == "GET" && r.URL.Path == "/arvados/v1/containers/"+apistub.container.UUID { + json.NewEncoder(w).Encode(apistub.container) + return + } + apistub.proxy.ServeHTTP(w, r) +} + func (s *TestSuite) TestLoadImage(c *C) { s.runner.Container.ContainerImage = arvadostest.DockerImage112PDH s.runner.Container.Mounts = map[string]arvados.Mount{ @@ -687,8 +752,9 @@ func (s *TestSuite) fullRunHelper(c *C, record string, extraMounts []string, fn } return d, err } + client, _ := apiStub() s.runner.MkArvClient = func(token string) (IArvadosClient, IKeepClient, *arvados.Client, error) { - return &ArvTestClient{secretMounts: secretMounts}, &s.testContainerKeepClient, nil, nil + return &ArvTestClient{secretMounts: secretMounts}, &s.testContainerKeepClient, client, nil } if extraMounts != nil && len(extraMounts) > 0 { @@ -1352,6 +1418,7 @@ func (s *TestSuite) TestSetupMounts(c *C) { cr := s.runner am := &ArvMountCmdLine{} cr.RunArvMount = am.ArvMountTest + cr.containerClient, _ = apiStub() cr.ContainerArvClient = &ArvTestClient{} cr.ContainerKeepClient = &KeepTestClient{} cr.Container.OutputStorageClasses = []string{"default"} @@ -1674,7 +1741,7 @@ func (s *TestSuite) TestSetupMounts(c *C) { { i = 0 cr.ArvMountPoint = "" - (*GitMountSuite)(nil).useTestGitServer(c) + git_client.InstallProtocol("https", git_http.NewClient(arvados.InsecureHTTPClient)) cr.token = arvadostest.ActiveToken cr.Container.Mounts = make(map[string]arvados.Mount) cr.Container.Mounts = map[string]arvados.Mount{ diff --git a/lib/crunchrun/git_mount.go b/lib/crunchrun/git_mount.go index 92bb6d11d9..561ea18de4 100644 --- a/lib/crunchrun/git_mount.go +++ b/lib/crunchrun/git_mount.go @@ -48,25 +48,22 @@ func (gm gitMount) validate() error { // ExtractTree extracts the specified tree into dir, which is an // existing empty local directory. -func (gm gitMount) extractTree(ac IArvadosClient, dir string, token string) error { +func (gm gitMount) extractTree(ac *arvados.Client, dir string, token string) error { err := gm.validate() if err != nil { return err } - baseURL, err := ac.Discovery("gitUrl") + dd, err := ac.DiscoveryDocument() if err != nil { - return fmt.Errorf("discover gitUrl from API: %s", err) - } else if _, ok := baseURL.(string); !ok { - return fmt.Errorf("discover gitUrl from API: expected string, found %T", baseURL) + return fmt.Errorf("error getting discovery document: %w", err) } - - u, err := url.Parse(baseURL.(string)) + u, err := url.Parse(dd.GitURL) if err != nil { - return fmt.Errorf("parse gitUrl %q: %s", baseURL, err) + return fmt.Errorf("parse gitUrl %q: %s", dd.GitURL, err) } u, err = u.Parse("/" + gm.UUID + ".git") if err != nil { - return fmt.Errorf("build git url from %q, %q: %s", baseURL, gm.UUID, err) + return fmt.Errorf("build git url from %q, %q: %s", dd.GitURL, gm.UUID, err) } store := memory.NewStorage() repo, err := git.Init(store, osfs.New(dir)) diff --git a/lib/crunchrun/git_mount_test.go b/lib/crunchrun/git_mount_test.go index e39beaa943..ac98dcc480 100644 --- a/lib/crunchrun/git_mount_test.go +++ b/lib/crunchrun/git_mount_test.go @@ -6,14 +6,11 @@ package crunchrun import ( "io/ioutil" - "net/url" "os" "path/filepath" - "git.arvados.org/arvados.git/lib/config" "git.arvados.org/arvados.git/sdk/go/arvados" "git.arvados.org/arvados.git/sdk/go/arvadostest" - "git.arvados.org/arvados.git/sdk/go/ctxlog" check "gopkg.in/check.v1" git_client "gopkg.in/src-d/go-git.v4/plumbing/transport/client" git_http "gopkg.in/src-d/go-git.v4/plumbing/transport/http" @@ -26,11 +23,10 @@ type GitMountSuite struct { var _ = check.Suite(&GitMountSuite{}) func (s *GitMountSuite) SetUpTest(c *check.C) { - s.useTestGitServer(c) - var err error s.tmpdir, err = ioutil.TempDir("", "") c.Assert(err, check.IsNil) + git_client.InstallProtocol("https", git_http.NewClient(arvados.InsecureHTTPClient)) } func (s *GitMountSuite) TearDownTest(c *check.C) { @@ -39,13 +35,14 @@ func (s *GitMountSuite) TearDownTest(c *check.C) { } // Commit fd3531f is crunch-run-tree-test -func (s *GitMountSuite) TestextractTree(c *check.C) { +func (s *GitMountSuite) TestExtractTree(c *check.C) { gm := gitMount{ Path: "/", UUID: arvadostest.Repository2UUID, Commit: "fd3531f42995344f36c30b79f55f27b502f3d344", } - err := gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) + ac := arvados.NewClientFromEnv() + err := gm.extractTree(ac, s.tmpdir, arvadostest.ActiveToken) c.Check(err, check.IsNil) fnm := filepath.Join(s.tmpdir, "dir1/dir2/file with mode 0644") @@ -85,7 +82,7 @@ func (s *GitMountSuite) TestExtractNonTipCommit(c *check.C) { UUID: arvadostest.Repository2UUID, Commit: "5ebfab0522851df01fec11ec55a6d0f4877b542e", } - err := gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) + err := gm.extractTree(arvados.NewClientFromEnv(), s.tmpdir, arvadostest.ActiveToken) c.Check(err, check.IsNil) fnm := filepath.Join(s.tmpdir, "file only on testbranch") @@ -100,7 +97,7 @@ func (s *GitMountSuite) TestNonexistentRepository(c *check.C) { UUID: "zzzzz-s0uqq-nonexistentrepo", Commit: "5ebfab0522851df01fec11ec55a6d0f4877b542e", } - err := gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) + err := gm.extractTree(arvados.NewClientFromEnv(), s.tmpdir, arvadostest.ActiveToken) c.Check(err, check.NotNil) c.Check(err, check.ErrorMatches, ".*repository not found.*") @@ -113,7 +110,7 @@ func (s *GitMountSuite) TestNonexistentCommit(c *check.C) { UUID: arvadostest.Repository2UUID, Commit: "bb66b6bb6b6bbb6b6b6b66b6b6b6b6b6b6b6b66b", } - err := gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) + err := gm.extractTree(arvados.NewClientFromEnv(), s.tmpdir, arvadostest.ActiveToken) c.Check(err, check.NotNil) c.Check(err, check.ErrorMatches, ".*object not found.*") @@ -127,8 +124,8 @@ func (s *GitMountSuite) TestGitUrlDiscoveryFails(c *check.C) { UUID: arvadostest.Repository2UUID, Commit: "5ebfab0522851df01fec11ec55a6d0f4877b542e", } - err := gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) - c.Check(err, check.ErrorMatches, ".*gitUrl.*") + err := gm.extractTree(&arvados.Client{}, s.tmpdir, arvadostest.ActiveToken) + c.Check(err, check.ErrorMatches, ".*error getting discovery doc.*") } func (s *GitMountSuite) TestInvalid(c *check.C) { @@ -186,7 +183,7 @@ func (s *GitMountSuite) TestInvalid(c *check.C) { matcher: ".*writable.*", }, } { - err := trial.gm.extractTree(&ArvTestClient{}, s.tmpdir, arvadostest.ActiveToken) + err := trial.gm.extractTree(arvados.NewClientFromEnv(), s.tmpdir, arvadostest.ActiveToken) c.Check(err, check.NotNil) s.checkTmpdirContents(c, []string{}) @@ -202,15 +199,3 @@ func (s *GitMountSuite) checkTmpdirContents(c *check.C, expect []string) { c.Check(err, check.IsNil) c.Check(names, check.DeepEquals, expect) } - -func (*GitMountSuite) useTestGitServer(c *check.C) { - git_client.InstallProtocol("https", git_http.NewClient(arvados.InsecureHTTPClient)) - - loader := config.NewLoader(nil, ctxlog.TestLogger(c)) - cfg, err := loader.Load() - c.Assert(err, check.IsNil) - cluster, err := cfg.GetCluster("") - c.Assert(err, check.IsNil) - - discoveryMap["gitUrl"] = (*url.URL)(&cluster.Services.GitHTTP.ExternalURL).String() -} -- 2.30.2