Made the adding new keyboard language support easier #669
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package com.cloud.agent.api; | ||
|
|
||
| public class CopyFileInVmAnswer extends Answer { | ||
| public CopyFileInVmAnswer() { | ||
| } | ||
|
|
||
| public CopyFileInVmAnswer(CopyFileInVmCommand cmd) { | ||
| super(cmd, true, null); | ||
| } | ||
|
|
||
| public CopyFileInVmAnswer(CopyFileInVmCommand cmd, String details) { | ||
| super(cmd, false, details); | ||
| } | ||
|
|
||
| public CopyFileInVmAnswer(CopyFileInVmCommand cmd, Exception e) { | ||
| super(cmd, false, e.getMessage()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package com.cloud.agent.api; | ||
|
|
||
|
|
||
| public class CopyFileInVmCommand extends Command { | ||
| String src; | ||
| String dest; | ||
| String vmIp; | ||
|
|
||
| public CopyFileInVmCommand(String src, String dest, String vmIp) { | ||
| super(); | ||
| this.src = src; | ||
| this.dest = dest; | ||
| this.vmIp = vmIp; | ||
| } | ||
|
|
||
| public String getSrc() { | ||
| return src; | ||
| } | ||
|
|
||
| public String getDest() { | ||
| return dest; | ||
| } | ||
|
|
||
| public String getVmIp() { | ||
| return vmIp; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean executeInSequence() { | ||
| return false; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,9 @@ | |
| import java.security.UnrecoverableKeyException; | ||
| import java.security.cert.CertificateException; | ||
| import java.security.cert.X509Certificate; | ||
|
|
||
| import org.apache.commons.io.FileUtils; | ||
| import org.apache.commons.io.filefilter.TrueFileFilter; | ||
| import org.joda.time.Duration; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
|
|
@@ -70,6 +73,8 @@ | |
| import com.cloud.agent.api.CheckS2SVpnConnectionsAnswer; | ||
| import com.cloud.agent.api.CheckS2SVpnConnectionsCommand; | ||
| import com.cloud.agent.api.Command; | ||
| import com.cloud.agent.api.CopyFileInVmAnswer; | ||
| import com.cloud.agent.api.CopyFileInVmCommand; | ||
| import com.cloud.agent.api.GetDomRVersionAnswer; | ||
| import com.cloud.agent.api.GetDomRVersionCmd; | ||
| import com.cloud.agent.api.GetVmConfigAnswer; | ||
|
|
@@ -491,6 +496,8 @@ public final Answer executeRequest(final Command cmd) { | |
| answer = execute((UnPlugNicCommand)cmd); | ||
| } else if (clazz == CopyCommand.class) { | ||
| answer = execute((CopyCommand)cmd); | ||
| } else if (clazz == CopyFileInVmCommand.class) { | ||
| answer = execute((CopyFileInVmCommand)cmd); | ||
| } | ||
| else { | ||
| if (clazz == StartCommand.class) { | ||
|
|
@@ -595,6 +602,25 @@ private PlugNicAnswer execute(final PlugNicCommand cmd) { | |
| } | ||
| } | ||
|
|
||
| private CopyFileInVmAnswer execute(CopyFileInVmCommand cmd) { | ||
| File keyFile = getSystemVMKeyFile(); | ||
| try { | ||
| File file = new File(cmd.getSrc()); | ||
| if(file.exists()) { | ||
|
Member
There was a problem hiding this comment. What about instantiating the response right away what do you think of that? |
||
| if(file.isDirectory()) { | ||
| for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) { | ||
| SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null); | ||
| } | ||
| } else { | ||
| SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. can you catch more specific exceptions here, please?
Author
There was a problem hiding this comment. SshHelper is throwing Exception so I am catching Exception. If you are talking about handling it here and not passing to CopyFileInVmAnswer then CopyFileInVmAnswer is just passing the exception message and not full exception. Applicable constructor from CopyFileInVmAnswer
Contributor
There was a problem hiding this comment. I understand you are basing yourself on bad code. That does not make it a good practice. Either the SshHelper should be refactored to some extend and the possible IOException should be caught seperately. See my general comment to this PR, please.
Author
There was a problem hiding this comment. @DaanHoogland I don't understand one point what difference it would make to catch specific exceptions and take same action in all case then catching generic exception.
Contributor
There was a problem hiding this comment. The message can be more specific and explanatory. The SshHelper shouldn't throw such a generic exception anyway. This is sloppy development and we now have a good chance to improve on it. |
||
| s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e); | ||
| return new CopyFileInVmAnswer(cmd, e); | ||
| } | ||
| return new CopyFileInVmAnswer(cmd); | ||
|
Contributor
There was a problem hiding this comment. Should this return be moved inside to end of try block?
Contributor
There was a problem hiding this comment. I mean in case a copyfilein..if moved inside try block throws an exception , then the code return new...copyfile inside catch still works.
Author
There was a problem hiding this comment. I got your point but I don't understand how will that make any difference here
Contributor
There was a problem hiding this comment. Its not a big issue as such, but iam seeing like if there is an exception at liine 617: "return new CopyFileIn.. (cmd)", then catch will still throw your new object created. Again, its not a great issue to be worry about!
Author
There was a problem hiding this comment. That line cannot through exception. |
||
| } | ||
|
|
||
| private UnPlugNicAnswer execute(final UnPlugNicCommand cmd) { | ||
| if (s_logger.isInfoEnabled()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package com.cloud.hypervisor.kvm.resource.wrapper; | ||
|
|
||
| import com.cloud.agent.api.Answer; | ||
| import com.cloud.agent.api.CopyFileInVmAnswer; | ||
| import com.cloud.agent.api.CopyFileInVmCommand; | ||
| import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; | ||
| import com.cloud.resource.CommandWrapper; | ||
| import com.cloud.resource.ResourceWrapper; | ||
| import com.cloud.utils.ssh.SshHelper; | ||
| import org.apache.commons.io.FileUtils; | ||
| import org.apache.commons.io.filefilter.TrueFileFilter; | ||
| import org.apache.log4j.Logger; | ||
|
|
||
| import java.io.File; | ||
|
|
||
| @ResourceWrapper(handles = CopyFileInVmCommand.class) | ||
| public class LibvirtCopyFileInVmCommandWrapper extends CommandWrapper<CopyFileInVmCommand, Answer, LibvirtComputingResource> { | ||
|
|
||
| private static final Logger s_logger = Logger.getLogger(LibvirtCopyFileInVmCommandWrapper.class); | ||
|
|
||
| @Override public Answer execute(CopyFileInVmCommand cmd, LibvirtComputingResource libvirtComputingResource) { | ||
| final File keyFile = new File("/root/.ssh/id_rsa.cloud"); | ||
| try { | ||
| File file = new File(cmd.getSrc()); | ||
|
Contributor
There was a problem hiding this comment. This code seems repetitive, can this be made as a lib, or convert to a factory based upon given hypervisor type and use that pattern in each command execute call?
Author
There was a problem hiding this comment. @sedukull Could you please elaborate more on what is repetitive here?
Contributor
There was a problem hiding this comment. I mean the below code is used across few code files in same form or some modified form, thats where i made a note, to avoid duplication. If its possible to make one common definition and use across.
Author
There was a problem hiding this comment. This is hypervisor specific code and each hypervisor is achieving it differently. To me it doesn't make sense to put it in lib or something like that as it will unnecessarily add complexity without achieving much.
Member
There was a problem hiding this comment. @anshul1886 please reduce these duplicated lines.
Author
There was a problem hiding this comment. @rafaelweingartner No, that's not the case. Some hypervisors copy through hypervisor and some directly copies to system vm. Also can you give example how all this can be achieved through common code. |
||
| if(file.exists()) { | ||
| if(file.isDirectory()) { | ||
| for (File f : FileUtils.listFiles(new File(cmd.getSrc()), TrueFileFilter.INSTANCE, TrueFileFilter.INSTANCE)) { | ||
| SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), f.getCanonicalPath(), null); | ||
| } | ||
| } else { | ||
|
Contributor
There was a problem hiding this comment. If its not a directory, but can be a block or socket file possible? Is there any criteria check to be added for file type to be copied?
Author
There was a problem hiding this comment. If its just a file then else part will take care. And I don't understand how does socket file is involved here.
Contributor
There was a problem hiding this comment. I mean we are checking whether its directory or not, but if its not a directory, other types can come EX: block, socket, or other file types defined by attributes ( like d,b etc), i mean do we have a specific requirement to copy with specific file type. EX: if we have group of files ( directory, block type, socket type , plain text files etc) , with above logic it returns all which is not of directory "type", iam inquiring to see we want to be explicit in code for a given file type?
Author
There was a problem hiding this comment. Here if it is a file then I am copying it directly as can be seen in else block but if it is a directory then I am listing files and then copying files individually as the library which we are using elsewhere in CloudStack doesn't support copying directories.
Contributor
There was a problem hiding this comment. My note was related to the below example.
Author
There was a problem hiding this comment. This is written mainly for copying js files to CPVM. And those js needs to be present on cloudstack installation. If somebody is able to manipulate CloudStack installation machine then that is major issue and we can't do anything about that. This makes me to believe that it doesn't makes much sense to do some file validation. |
||
| SshHelper.scpTo(cmd.getVmIp(), 3922, "root", keyFile, null, cmd.getDest(), file.getCanonicalPath(), null); | ||
| } | ||
| } | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. please, catch more specific exceptions. |
||
| s_logger.error("Fail to copy file " + cmd.getSrc() + " in VM " + cmd.getVmIp(), e); | ||
| return new CopyFileInVmAnswer(cmd, e); | ||
| } | ||
| return new CopyFileInVmAnswer(cmd); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any unit-tests added for this change? As well, if we possible, can we have a document for the console proxy workflow available on wiki ? This may help people to know the exact workflow or class interactions with business flow.
As well, a small change to this request message "Made the adding new keyboard language support easier" could be beneficial i believe, helps people to know the complete context for this change.
Note :this is not specific to this line, but a generic note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FS https://cwiki.apache.org/confluence/display/CLOUDSTACK/Support+for+non-US+keyboards+in+Console+Proxy has details regarding console proxy workflow which is present in description.
In developer setup these files are always present so adding unit test for certain (which will always pass) thing doesn't make sense to me. In my opinion unit test are meant to test corner cases or some uncertainty.
I will try to improve description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the FS link, it was helpful. Regarding the unit test, i would disagree to say that they are meant to test corner cases. Not sure, what it doesnt make sense to you. In general, unit test purpose is not what you meant, in this commit, if we see we modified near to 28 code files, added new classes and code, it makes sense to have unit tests for the added code. It will help to test the code units and individual functional blocks during build, these areas not always are touched by functional tests. It adds to the coverage, make sure code modified down the lane adheres to the contract through failures in tests etc. Its appreciated, if we can have few unit tests added to this, right now they are zero tests for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding about unit testing is similar to mentioned in this blog http://simpleprogrammer.com/2010/10/15/the-purpose-of-unit-testing/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its good that it does not say that UT are meant to test "corner" cases or "some uncertainity", as per your earlier understanding. It talks about TDD, in which UT are not meant to be written post the code, its good to follow TDD if we can write tests before the code. But if we missed it, and If we feel there is a value add in writing some tests, which can benefit others and helps testing the code in long run, its appreciated to add few, there is no harm. We could see many commits coming are having UT recently. Some even made sure to get near 100% coverage for their code. We know its not a new distinct module, its up to you and whatever the community decides!!!