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

shell.fetch delivers files to an incorrect location #95

Closed
elutfall opened this Issue Sep 14, 2011 · 16 comments

Comments

Projects
None yet
2 participants
@elutfall

elutfall commented Sep 14, 2011

Summary: When using shell.fetch(source, dest) if you don't put a leading slash on the destination it puts the file in the wrong location.

Workaround: Always include the leading slash when specifying a fetch destination.

host names and filenames obscured to protect the guilty.

Agent 3.2.0 on CentOS 6, installed using defaults at /opt/glu

/opt/glu/agent-server/3.2.0/conf/pre_master_conf.sh:

GLU_ZOOKEEPER="zookeeper.foo.com:2181"
GLU_AGENT_FABRIC="glu_demo"
GLU_AGENT_APPS="/opt/apps"
JAVA_HOME="/usr/local/java/jdk1.6"

Snippet from glu script:

    def configure = {
        shell.fetch("${params.overrides.source}", "${params.overrides.destination}")
    }

Snippet from JSON:

   {
      "agent": "agent.foo.com",
      "initParameters": {
            "foo": "bar"
        },
        "overrides": {
          "destination": "services/conf/test.xml",
          "source": "http://foo.com/orbitz/services/conf/test.xml"
        },
      },
      "mountPoint": "/webapps/0",
      "script": "http://console.foo.com:8080/glu/repository/scripts/hostinstall_test.groovy",
    },

What I would expect this to do is to put text.xml in /opt/apps/services/conf/test.xml
It actually puts it in /opt/apps/opt/glu/agent-server/3.2.0/service/conf/text.xml

If "destination" is specified with a leading slash, it goes to the right place.

So what's happening if you don't put the leading slash in, is that it puts the file in
${GLU_AGENT_APPS}/${GLU_AGENT_HOME}/${params.overrides.destination}
instead of
${GLU_AGENT_APPS}/${params.overrides.destination}

It silently creates the GLU_AGENT_HOME tree under GLU_AGENT_APPS.

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 14, 2011

Hmmm.. clearly from the description it looks like a bug! I will investigate.

@ghost ghost assigned ypujante Sep 14, 2011

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 14, 2011

I cannot reproduce the problem. Here is what I have done:

in pre_master_conf.sh

GLU_AGENT_APPS="/export/content/glu/glu-95/apps"

Then I created a small test script:

class TestScript {
  def install = {
    log.info "executing fetch"
    def a = shell.fetch("file:///tmp/TestScript.groovy", "foo/toto.groovy")
    log.info a?.toString()
  }
}

Then I use the cli to install/run the script:

./bin/agent-cli.sh -v -m /test -I file:///tmp/TestScript.groovy

This is what I see in the agent log file (shell.fetch returns the destination):

2011/09/14 12:03:00.986 INFO [/test] executing fetch
2011/09/14 12:03:01.032 INFO [/test] file:/export/content/glu/glu-95/apps//foo/toto.groovy

Then I confirm that the file has been created in its proper location:

ls -la /export/content/glu/glu-95/apps//foo/toto.groovy
-rw-r--r--  1 ypujante  admin  176 Sep 14 12:03 /export/content/glu/glu-95/apps//foo/toto.groovy

I am very confused as it is clearly working as intended and I don't seem to be able to reproduce the issue.

Would you mind trying with the same test script and the agent cli?

Thanks

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

I'll give it a shot tomorrow. We are using the console-server. I know that it just triggers the agent-server, and the cli does the same, so there should be no difference. Can you test with the gui?

What is your GLU_AGENT_HOME in this example? I see that your file path has a double slash in it in between GLU_AGENT_APPS and the fetch destination. Even if I can't replicate my issue using the cli, I would think the '//' is an indication of something weird in the way shell.fetch() constructs its destination path.

Thanks,

Eli

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

OK, I was not able to reproduce with the cli or the console today. I replicated many times yesterday - I even did a complete reinstall and had the same problem. The only change I can see is that one of our devs has been tweaking our glu script and JSON to add extra functionality. The only difference is that we now iterate over a list of files to fetch.

This works with or without the leading slash.

  def configure = {
    params.overrides.each {
      shell.fetch(it.source, it.destination)
    }
  }

I'll put things back to try to replicate, but its looking like it was a bug in our glu script/

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 15, 2011

Ok. I will close this bug now. If you are able to reproduce, simply reopen it with the details.
Thanks

@ypujante ypujante closed this Sep 15, 2011

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

OK, I was able to replicate again. It's an interesting issue.

This code puts the dest in GLU_AGENT_APPS

  def configure = {
    shell.fetch(params.overrides.source, params.overrides.destination)

  }

This code puts the destination in $GLU_AGENT_APPS/$GLU_AGENT_HOME

  def configure = {
    shell.fetch("${params.overrides.source}", "${params.overrides.destination}")

  }

But it only does it if your destination does not have a leading slash.

Not sure how we snuck in the ${}, but that's the big part of the issue. Not sure if this could still be considered a bug, but it is a weird case. The GLU_AGENT_HOME addition doesn't happen elsewhere. For instance:
def cmd = "/opt/apps/bin/install.py -h ${params.artifact}"
works fine.

I leave it to you if you still want to consider this a bug.

Thanks for your timely responses, as always,

Eli

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

Here's a use case for that bug. If I want to prepend a path to the destination in the glu script, I would need to put the destination in quotes.

  def configure = {
    shell.fetch(params.overrides.source, "foo/${params.overrides.destination}")

  }

In this case, the file goes to $GLU_AGENT_APPS/$GLU_AGENT_HOME/foo/${params.overrides.destination}

  def configure = {
    shell.fetch(params.overrides.source, "/foo/${params.overrides.destination}")

  }

In this case, the file goes to $GLU_AGENT_APPS/foo/${params.overrides.destination}

@ypujante ypujante reopened this Sep 15, 2011

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 15, 2011

Can you try with

def configure = {
  shell.fetch(params.overrides.source, "foo/${params.overrides.destination}".toString())

}

Does it make a difference?

I am reopening the issue as I think it is related to some weirdness with the way groovy handles GStrings vs String and the code does not seem to handle it properly.

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

Replicated with the shell.mv() method as well.

shell.mv(it.destination, "/foo/${it.destination}")

Goes to GLU_AGENT_APPS/foo/destinaton

shell.mv(it.destination, "foo/${it.destination}")

Goes to GLU_AGENT_APPS/GLU_AGENT_HOME/foo/destination

This would be a much more common use case as this might be the way someone backs up a file, rotates a log, etc.

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 15, 2011

All methods under the cover use: shell.toResource(xxx) so it is most likely this one which is broken. Try

log.info shell.toResource("/foo/${it.destination}")
log.info shell.toResource("foo/${it.destination}")
log.info shell.toResource("foo/${it.destination}".toString())
@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

toString() worked.

Maybe just document that you should always use a leading slash for now.

@elutfall

This comment has been minimized.

elutfall commented Sep 15, 2011

All of those give an error like this:

* script=GluScript [/apps/0], action=configure
* [groovy.lang.MissingMethodException]: No signature of method: org.slf4j.impl.Log4jLoggerAdapter.info() is applicable for argument types: (org.linkedin.groovy.util.io.fs.SerializableFileResource) values: [file:/opt/apps/foo/services/conf/test.xml] Possible solutions: info(java.lang.String), info(org.slf4j.Marker, java.lang.String), info(java.lang.String, [Ljava.lang.Object;), info(java.lang.String, java.lang.Object), info(java.lang.String, java.lang.Throwable), info(org.slf4j.Marker, java.lang.String, [Ljava.lang.Object;)

I'm guessing it's because we're trying to log an object instead of a string.

This logs fine: log.info "${it.destination}"

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 20, 2011

Ok I am able to reproduce locally now:

class TestScript
{
 def install = {
 def path1 = "/foo/toto.xml"
 def path2 = "foo/toto.xml"

 log.info shell.toResource(path1).toString()
 log.info shell.toResource("${path1}").toString()
 log.info shell.toResource(path2).toString()
 log.info shell.toResource("${path2}").toString()
 }
}

gives this output:

2011/09/20 10:52:39.390 INFO [/test] file:/export/content/glu/glu-95/foo/toto.xml
2011/09/20 10:52:39.391 INFO [/test] file:/export/content/glu/glu-95/foo/toto.xml
2011/09/20 10:52:39.392 INFO [/test] file:/export/content/glu/glu-95/foo/toto.xml
2011/09/20 10:52:39.393 INFO [/test] file:/export/content/glu/glu-95/export/content/glu/org.linkedin.glu.packaging-all-3.3.0/agent-server/3.3.0/foo/toto.xml

Clearly the last entry is wrong (and it should be the same as entry 3!)

Yan

@ypujante

This comment has been minimized.

Member

ypujante commented Sep 20, 2011

I found and fixed the issue in the linkedin-utils project (linkedin/linkedin-utils#4). It will be fixed in the next version of glu.

@elutfall

This comment has been minimized.

elutfall commented Sep 28, 2011

Awesome. Thanks!

@ypujante

This comment has been minimized.

Member

ypujante commented Oct 12, 2011

fixed with 3.4.0

@ypujante ypujante closed this Oct 12, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment